On Wed, 17.07.13 13:01, Maciej Wereski (m.were...@partner.samsung.com) wrote:
> This allows to show only units with specified SUB or ACTIVE state. > > Signed-off-by: Maciej Wereski <m.were...@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. _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel