[systemd-devel] [PATCH 2/4] systemctl: edit: improve error messages, report actual error for units which could not be loaded

[Zbigniew Jędrzejewski-Szmek]

This is a minor thing, but makes things easier, especially when using
gitk or git format-patch: please keep the patch subject at around 80 characters
(<=72 is best). I know we don't always to this, sometimes it just isn't possible,
but in this case something like 'systemctl: report actual load error' would
convey essentially the same information.

It is also nice to describe the change in present tense (i.e. "Mumble
this, make foo do bar"). The past tenses can than be used to describe
status quo ante.

On Fri, Dec 19, 2014 at 05:08:08PM +0300, Ivan Shapovalov wrote:
> "Not found" condition in find_paths_to_edit() has been made non-fatal
> because the error message in edit() ("Cannot find any units to edit")
> suggests that.
> 
> Error messages in edit() themselves have been removed because
> - result of expand_names() is not checked anywhere else
> - an extra "cannot find any units to edit" is redundant due to error reporting
>   for each unit in unit_find_paths() and find_paths_to_edit()

We don't want to silently fail if units to edit are not found.
If the user says 'edit this that', and 'this' is not found, she
should get an error. Then she can fix the typo and reexecute the
command. But if we continue and launch the editor, the error is
hidden, which is confusing. There's no reason not to fail
immediately.

>  src/systemctl/systemctl.c | 53 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
> index 7af111c..658793e 100644
> --- a/src/systemctl/systemctl.c
> +++ b/src/systemctl/systemctl.c
> @@ -2325,9 +2325,12 @@ static int unit_find_paths(sd_bus *bus,
>  
>          if (!avoid_bus_cache && !unit_name_is_template(unit_name)) {
>                  _cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
> +                _cleanup_bus_message_unref_ sd_bus_message *unit_load_error = NULL;
>                  _cleanup_free_ char *unit = NULL;
>                  _cleanup_free_ char *path = NULL;
>                  _cleanup_strv_free_ char **dropins = NULL;
> +                _cleanup_strv_free_ char **load_error = NULL;
> +                char *unit_load_error_name, *unit_load_error_message;
>  
>                  unit = unit_dbus_path_from_name(unit_name);
>                  if (!unit)
> @@ -2336,6 +2339,31 @@ static int unit_find_paths(sd_bus *bus,
>                  if (need_daemon_reload(bus, unit_name) > 0)
>                          warn_unit_file_changed(unit_name);
>  
> +                r = sd_bus_get_property(
> +                                bus,
> +                                "org.freedesktop.systemd1",
> +                                unit,
> +                                "org.freedesktop.systemd1.Unit",
> +                                "LoadError",
> +                                &error,
> +                                &unit_load_error,
> +                                "(ss)");
> +                if (r < 0)
> +                        return log_error_errno(r, "Failed to get LoadError: %s", bus_error_message(&error, r));
> +
> +                r = sd_bus_message_read(
> +                                unit_load_error,
> +                                "(ss)",
> +                                &unit_load_error_name,
> +                                &unit_load_error_message);
> +                if (r < 0)
> +                        return bus_log_parse_error(r);
> +
> +                if (!isempty(unit_load_error_name)) {
> +                        log_error("Unit %s is not loaded, ignoring: %s", unit_name, unit_load_error_message);
> +                        return 0;
> +                }
> +
>                  r = sd_bus_get_property_string(
>                                  bus,
>                                  "org.freedesktop.systemd1",
> @@ -2403,6 +2431,10 @@ static int unit_find_paths(sd_bus *bus,
>                          r = unit_file_find_dropin_paths(lp->unit_path, NULL, names, dropin_paths);
>          }
>  
> +        if (r == 0) {
> +                log_error("No files found for unit %s, ignoring.", unit_name);
> +        }
Those parenthesese are unnecessary.

> +
>          return r;
>  }
>  
> @@ -4780,10 +4812,8 @@ static int cat(sd_bus *bus, char **args) {
>                  r = unit_find_paths(bus, *name, avoid_bus_cache, &lp, &fragment_path, &dropin_paths);
>                  if (r < 0)
>                          return r;
> -                else if (r == 0) {
> -                        log_warning("Unit %s does not have any files on disk", *name);
> +                else if (r == 0)
>                          continue;
> -                }
>  
>                  if (first)
>                          first = false;
> @@ -6114,9 +6144,13 @@ static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) {
>                  r = unit_find_paths(bus, *name, avoid_bus_cache, &lp, &path, NULL);
>                  if (r < 0)
>                          return r;
> -                else if (r == 0 || !path)
> +                else if (r == 0)
> +                        continue;
> +                else if (!path) {
>                          // FIXME: support units with path==NULL (no FragmentPath)
> -                        return log_error_errno(ENOENT, "Unit %s not found, cannot edit.", *name);
> +                        log_error("No fragment exists for unit %s, ignoring.");
arg is missing here.

> +                        continue;
> +                }
>  
>                  if (arg_full)
>                          r = unit_file_create_copy(*name, path, user_home, user_runtime, &new_path, &tmp_path);
> @@ -6155,19 +6189,12 @@ static int edit(sd_bus *bus, char **args) {
>          if (r < 0)
>                  return log_error_errno(r, "Failed to expand names: %m");
>  
> -        if (!names) {
> -                log_error("No unit name found by expanding names");
> -                return -ENOENT;
> -        }
> -
>          r = find_paths_to_edit(bus, names, &paths);
>          if (r < 0)
>                  return r;
>  
> -        if (strv_isempty(paths)) {
> -                log_error("Cannot find any units to edit");
> +        if (strv_isempty(paths))
>                  return -ENOENT;
> -        }
>  
>          r = run_editor(paths);

Zbyszek