From 50f20d1bc2211a0e769278471aac108aa79ab26e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Sun, 24 May 2020 13:47:53 +0200
Subject: [PATCH] busctl: verify args early and always print results to stdout

We would print the error sometimes to stdout and sometimes to stderr. It *is*
useful to get the message if one of the names is not found on the bus to
stdout, so that this shows out in the pager. So let's do verification of args
early to catch invalid arguments, and then if we receive an error over the bus
(most likely that the name is not activatable), let's print to stdout so it
gets paged. E.g. 'busctl tree org.freedesktop.systemd1 org.freedesktop.systemd2'
gives a nicely usable output.
---
 TODO                |  3 ---
 src/busctl/busctl.c | 31 +++++++++++++++++--------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/TODO b/TODO
index e4182588fe..e962db26ca 100644
--- a/TODO
+++ b/TODO
@@ -4,9 +4,6 @@ Bugfixes:
   manager or system manager can be always set. It would be better to reject
   them when parsing config.
 
-* busctl prints errors to stdout:
-  busctl tree org.freedesktop.systemd1  /org/freedesktop/systemd1
-
 External:
 
 * Fedora: add an rpmlint check that verifies that all unit files in the RPM are listed in %systemd_post macros.
diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c
index a4132b3345..4879b466e3 100644
--- a/src/busctl/busctl.c
+++ b/src/busctl/busctl.c
@@ -462,7 +462,7 @@ static int on_path(const char *path, void *userdata) {
         return 0;
 }
 
-static int find_nodes(sd_bus *bus, const char *service, const char *path, Set *paths, bool many) {
+static int find_nodes(sd_bus *bus, const char *service, const char *path, Set *paths) {
         static const XMLIntrospectOps ops = {
                 .on_path = on_path,
         };
@@ -476,12 +476,10 @@ static int find_nodes(sd_bus *bus, const char *service, const char *path, Set *p
                                "org.freedesktop.DBus.Introspectable", "Introspect",
                                &error, &reply, "");
         if (r < 0) {
-                if (many)
-                        printf("Failed to introspect object %s of service %s: %s\n",
-                               path, service, bus_error_message(&error, r));
-                else
-                        log_error_errno(r, "Failed to introspect object %s of service %s: %s",
-                                        path, service, bus_error_message(&error, r));
+                printf("%sFailed to introspect object %s of service %s: %s%s\n",
+                       ansi_highlight_red(),
+                       path, service, bus_error_message(&error, r),
+                       ansi_normal());
                 return r;
         }
 
@@ -492,7 +490,7 @@ static int find_nodes(sd_bus *bus, const char *service, const char *path, Set *p
         return parse_xml_introspect(path, xml, &ops, paths);
 }
 
-static int tree_one(sd_bus *bus, const char *service, const char *prefix, bool many) {
+static int tree_one(sd_bus *bus, const char *service, const char *prefix) {
         _cleanup_set_free_ Set *paths = NULL, *done = NULL, *failed = NULL;
         _cleanup_free_ char **l = NULL;
         int r;
@@ -521,7 +519,7 @@ static int tree_one(sd_bus *bus, const char *service, const char *prefix, bool m
                     set_contains(failed, p))
                         continue;
 
-                q = find_nodes(bus, service, p, paths, many);
+                q = find_nodes(bus, service, p, paths);
                 if (q < 0 && r >= 0)
                         r = q;
 
@@ -550,6 +548,12 @@ static int tree(int argc, char **argv, void *userdata) {
         char **i;
         int r = 0;
 
+        /* Do superficial verification of arguments before even opening the bus */
+        STRV_FOREACH(i, strv_skip(argv, 1))
+                if (!sd_bus_service_name_is_valid(*i))
+                        return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
+                                               "Invalid bus service name: %s", *i);
+
         if (!arg_unique && !arg_acquired)
                 arg_acquired = true;
 
@@ -581,14 +585,14 @@ static int tree(int argc, char **argv, void *userdata) {
 
                         printf("Service %s%s%s:\n", ansi_highlight(), *i, ansi_normal());
 
-                        q = tree_one(bus, *i, NULL, true);
+                        q = tree_one(bus, *i, NULL);
                         if (q < 0 && r >= 0)
                                 r = q;
 
                         not_first = true;
                 }
-        } else {
-                STRV_FOREACH(i, argv+1) {
+        } else
+                STRV_FOREACH(i, strv_skip(argv, 1)) {
                         int q;
 
                         if (i > argv+1)
@@ -599,11 +603,10 @@ static int tree(int argc, char **argv, void *userdata) {
                                 printf("Service %s%s%s:\n", ansi_highlight(), *i, ansi_normal());
                         }
 
-                        q = tree_one(bus, *i, NULL, !!argv[2]);
+                        q = tree_one(bus, *i, NULL);
                         if (q < 0 && r >= 0)
                                 r = q;
                 }
-        }
 
         return r;
 }