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

[Lennart Poettering]

On Fri, 19.12.14 17:08, Ivan Shapovalov (intelfx100 at gmail.com) wrote:

What happened to this patch series actually? I think only 1/4 was ever
commited, what about the other ones? Ivan, any chance you can rebase
the rest with Zbigniew's requested changes and post again?

Thanks,

Lennart

> "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()
> ---
>  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);
> +        }
> +
>          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.");
> +                        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);
>          if (r < 0)
> -- 
> 2.2.0
> 
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Lennart

-- 
Lennart Poettering, Red Hat