Re: [systemd-devel] [PATCH 2/4] systemctl: edit: improve error messages, report actual error for units which could not be loaded
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, &pat
Re: [systemd-devel] [PATCH 2/4] systemctl: edit: improve error messages, report actual error for units which could not be loaded
On Fri, 19.12.14 17:08, Ivan Shapovalov (intelfx...@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_f
Re: [systemd-devel] [PATCH 2/4] systemctl: edit: improve error messages, report actual error for units which could not be loaded
On 2015-02-03 at 23:05 +0100, Lennart Poettering wrote: > On Fri, 19.12.14 17:08, Ivan Shapovalov (intelfx...@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? Yeah, I've kinda forgotten about this series... Will do. Thanks for the reminder! -- Ivan Shapovalov / intelfx / signature.asc Description: This is a digitally signed message part ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel