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

[Lennart Poettering]

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 

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

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

[Maciej Wereski]

This allows to show only units with specified SUB or ACTIVE state.

Signed-off-by: Maciej Wereski 
---
 man/systemctl.xml | 15 +--
 src/systemctl/systemctl.c | 43 +--
 2 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/man/systemctl.xml b/man/systemctl.xml
index f550215..2fb74c5 100644
--- a/man/systemctl.xml
+++ b/man/systemctl.xml
@@ -114,6 +114,16 @@ along with systemd; If not, see 
.
   
 
   
+--state=
+
+
+Argument should be a comma-separated list of unit
+SUB or ACTIVE states. When listing units show only those
+with specified SUB or ACTIVE state.
+
+  
+
+  
 -p
 --property=
 
@@ -169,8 +179,9 @@ along with systemd; If not, see 
.
 --failed
 
 
-  When listing units, show only failed units. Do not
-  confuse with --fail.
+  When listing units, show only failed units.
+  This is the same as --state=failed.
+  Do not confuse with --fail.
 
   
 
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 9574ff2..61b4730 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -93,13 +93,13 @@ static bool arg_quiet = false;
 static bool arg_full = false;
 static int arg_force = 0;
 static bool arg_ask_password = true;
-static bool arg_failed = false;
 static bool arg_runtime = false;
 static char **arg_wall = NULL;
 static const char *arg_kill_who = NULL;
 static int arg_signal = SIGTERM;
 static const char *arg_root = NULL;
 static usec_t arg_when = 0;
+static char **arg_state = NULL;
 static enum action {
 ACTION_INVALID,
 ACTION_SYSTEMCTL,
@@ -301,8 +301,8 @@ static int compare_unit_info(const void *a, const void *b) {
 static bool output_show_unit(const struct unit_info *u) {
 const char *dot;
 
-if (arg_failed)
-return streq(u->active_state, "failed");
+if (!strv_isempty(arg_state))
+return strv_contains(arg_state, u->sub_state) || 
strv_contains(arg_state, u->active_state);
 
 return (!arg_types || ((dot = strrchr(u->id, '.')) &&
strv_find(arg_types, dot+1))) &&
@@ -4660,12 +4660,13 @@ static int systemctl_help(void) {
"  -h --help   Show this help\n"
" --versionShow package version\n"
"  -t --type=TYPE  List only units of a particular type\n"
+   " --state=STATEShow only units with particular SUB or 
ACTIVE state\n"
"  -p --property=NAME  Show only properties by this name\n"
"  -a --allShow all loaded units/properties, 
including dead/empty\n"
"  ones. To list all units installed on the 
system, use\n"
"  the 'list-unit-files' command instead.\n"
" --reverseShow reverse dependencies with 
'list-dependencies'\n"
-   " --failed Show only failed units\n"
+   " --failed Show only failed units (the same as 
--state=failed)\n"
"  -l --full   Don't ellipsize unit names on output\n"
" --fail   When queueing a new job, fail if 
conflicting jobs are\n"
"  pending\n"
@@ -4886,7 +4887,8 @@ static int systemctl_parse_argv(int argc, char *argv[]) {
 ARG_FAILED,
 ARG_RUNTIME,
 ARG_FORCE,
-ARG_PLAIN
+ARG_PLAIN,
+ARG_STATE
 };
 
 static const struct option options[] = {
@@ -4925,6 +4927,7 @@ static int systemctl_parse_argv(int argc, char *argv[]) {
 { "lines", required_argument, NULL, 'n'   },
 { "output",required_argument, NULL, 'o'   },
 { "plain", no_argument,   NULL, ARG_PLAIN },
+{ "state", required_argument, NULL, ARG_STATE },
 { NULL,0, NULL, 0 }
 };
 
@@ -5086,7 +5089,14 @@ static int systemctl_parse_argv(int argc, char *argv[]) {
 break;
 
 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;
+}
+}
 break;
 
 case 'q':
@@ -5156,6 +5166,27 @@ st