[systemd-devel] [PATCHv3] systemctl, man: option to list units by state

[Lennart Poettering]

On Wed, 17.07.13 13:01, Maciej Wereski (m.wereski at partner.samsung.com) wrote:

> This allows to show only units with specified SUB or ACTIVE state.
> 
> Signed-off-by: Maciej Wereski <m.wereski at partner.samsung.com>

Hmm, could you make --state= check all three states please? i.e. active,
sub and load state equally. And leave --type= as it is right now for
compat reasons meaning you can check the load state via both --state=
and --type= that way.

Then, I'd like to see --failed and the use of --type= for filtering for
load state deprecated, even if we continue to support them. We usually
do that simply by removing them from the documentation and --help texts,
and just leaving them in as "secret" compat features...

With all that in place we'd not expose any redundancy to the user. We'd
have --state= for checking for any of the three states, and we'd have
--type= for checking for unit types, and that'd (officially) be all we
expose.

Can I convince you to update your patch accordingly? i.e. removal of
--fail and the load state bits of --type= from the man page and --help,
and make --state= check work for load state too?

>                  case ARG_FAILED:
> -                        arg_failed = true;
> +                        if (!strv_contains(arg_state, "failed")) {
> +                                char **tmp = arg_state;
> +                                arg_state = strv_append(arg_state, "failed");
> +                                if (!arg_state) {
> +                                        strv_free(tmp);
> +                                        return -ENOMEM;

Consider using strv_extend() for this, which makes this code much shorter.

> +                        FOREACH_WORD_SEPARATOR(word, size, optarg, ",", state) {
> +                                char _cleanup_free_ *str;
> +
> +                                str = strndup(word, size);
> +                                if (!str)
> +                                        return -ENOMEM;
> +                                 if (!strv_contains(arg_state, str)) {
> +                                        char **tmp = arg_state;
> +                                        arg_state =
> strv_append(arg_state, str);

And for this one please use strv_push()!

strv_extend() and strv_push() have the same signatures. strv_extends()
just extends an existing strv array, and makes a copy of the passed in
string for that. This is what you want for adding the static const
string "failed" to it. And strv_push() also extends the strv array, but
takes possession of the string you pass. Which is great for the case
where you strndup()ed the string anyway.

Hope that makes sense, and thanks a lot for working on this and for
putting up with my awful review times for the original patch!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.