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_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
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
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, path,
NULL);
if (r 0)
return r;
-else if (r == 0 || !path)
+
