Discussion:
Using `systemctl edit` on "invalid" unit names
(too old to reply)
Ivan Shapovalov
2014-12-13 10:33:52 UTC
Permalink
Hello all,

it seems that the newly added `systemctl edit` command requires its arguments
to be valid unit names.

This causes `edit` operation to fail in apparently valid use-cases like

systemctl edit ***@.service
or
systemctl edit ***@tty1.service

In second case, the error message is especially cryptic:
"***@tty1.service ignored: not found".

Actually I understand what it does mean: systemctl asks the manager to show
unit's FragmentPath -> the manager tries to load the unit -> loading fails with
"File exists" because ***@tty1.service is already instantiated.

(BTW, it's a separate question: is this failure valid or is it a bug?)

But well. I guess that `edit` operation should always work with unit files
directly, just like enable/disable commands do.

Is this all correct? Can anyone please comment on these two issues?
--
Ivan Shapovalov / intelfx /
Ronny Chevalier
2014-12-13 14:34:01 UTC
Permalink
Post by Ivan Shapovalov
Hello all,
Hi,
Post by Ivan Shapovalov
it seems that the newly added `systemctl edit` command requires its arguments
to be valid unit names.
This causes `edit` operation to fail in apparently valid use-cases like
This is fixed in git now, thanks!
Post by Ivan Shapovalov
or
It worked before and it still works for me.
Post by Ivan Shapovalov
Actually I understand what it does mean: systemctl asks the manager to show
unit's FragmentPath -> the manager tries to load the unit -> loading fails with
I don't see why it should fail for this reason ?
Post by Ivan Shapovalov
(BTW, it's a separate question: is this failure valid or is it a bug?)
But well. I guess that `edit` operation should always work with unit files
directly, just like enable/disable commands do.
systemctl edit try to use the bus if it is possible, because this is
the only way you can know where is the unit file of the unit
foo.service currently running. Is it in /etc/systemd or
/usr/lib/systemd? If we check directly the file system we will assume
this is the first directory with the highest priority, which is wrong
in some cases.

But systemctl edit wanted to work with valid unit names which is wrong
in some cases too, so this is fixed now.
Post by Ivan Shapovalov
Is this all correct? Can anyone please comment on these two issues?
Thanks for the report!
Post by Ivan Shapovalov
--
Ivan Shapovalov / intelfx /
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Ivan Shapovalov
2014-12-14 13:21:32 UTC
Permalink
Post by Ronny Chevalier
Post by Ivan Shapovalov
Hello all,
Hi,
Post by Ivan Shapovalov
it seems that the newly added `systemctl edit` command requires its arguments
to be valid unit names.
This causes `edit` operation to fail in apparently valid use-cases like
This is fixed in git now, thanks!
Post by Ivan Shapovalov
or
It worked before and it still works for me.
Post by Ivan Shapovalov
Actually I understand what it does mean: systemctl asks the manager to show
unit's FragmentPath -> the manager tries to load the unit -> loading fails with
I don't see why it should fail for this reason ?
Post by Ivan Shapovalov
(BTW, it's a separate question: is this failure valid or is it a bug?)
Now both `edit getty@` and `edit ***@tty1` work, but I'd expect the latter
to honor the template parameter; i. e. create a drop-in for ***@tty1.service...
Is this possible?
--
Ivan Shapovalov / intelfx /
Zbigniew Jędrzejewski-Szmek
2014-12-16 05:35:09 UTC
Permalink
Post by Ivan Shapovalov
Post by Ronny Chevalier
Post by Ivan Shapovalov
Hello all,
Hi,
Post by Ivan Shapovalov
it seems that the newly added `systemctl edit` command requires its arguments
to be valid unit names.
This causes `edit` operation to fail in apparently valid use-cases like
This is fixed in git now, thanks!
Post by Ivan Shapovalov
or
It worked before and it still works for me.
Post by Ivan Shapovalov
Actually I understand what it does mean: systemctl asks the manager to show
unit's FragmentPath -> the manager tries to load the unit -> loading fails with
I don't see why it should fail for this reason ?
Post by Ivan Shapovalov
(BTW, it's a separate question: is this failure valid or is it a bug?)
Is this possible?
I made various unifications to the code to make it more maintainable. This
case should be fixed too. Please test it... it's easy to miss the corner cases.

Zbyszek
Ivan Shapovalov
2014-12-17 08:29:38 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
Post by Ronny Chevalier
Post by Ivan Shapovalov
Hello all,
Hi,
Post by Ivan Shapovalov
it seems that the newly added `systemctl edit` command requires its arguments
to be valid unit names.
This causes `edit` operation to fail in apparently valid use-cases like
This is fixed in git now, thanks!
Post by Ivan Shapovalov
or
It worked before and it still works for me.
Post by Ivan Shapovalov
Actually I understand what it does mean: systemctl asks the manager to show
unit's FragmentPath -> the manager tries to load the unit -> loading fails with
I don't see why it should fail for this reason ?
Post by Ivan Shapovalov
(BTW, it's a separate question: is this failure valid or is it a bug?)
Is this possible?
I made various unifications to the code to make it more maintainable. This
case should be fixed too. Please test it... it's easy to miss the corner cases.
Yes, that works now, thanks! Also the error messages are now a lot cleaner
and more "in line" with the rest of systemctl.

I still can't edit ***@tty1.service (other instances work correctly), but
I guess that's another problem. However, the error message could be improved
by querying the manager for LoadFailed and displaying that message instead
of just saying "does not have any files on disk". I'll try to come up with a patch
for that.

BTW, would it be good to have a LoadFailedErrno property or something in that line
to get an errno instead of a string? Or is it better to convert string to errno
using sd_bus_error_*() methods?

I'm sorry for too many questions :P

Thanks,
--
Ivan Shapovalov / intelfx /
Zbigniew Jędrzejewski-Szmek
2014-12-17 13:19:11 UTC
Permalink
Post by Ivan Shapovalov
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
Post by Ronny Chevalier
Post by Ivan Shapovalov
Hello all,
Hi,
Post by Ivan Shapovalov
it seems that the newly added `systemctl edit` command requires its arguments
to be valid unit names.
This causes `edit` operation to fail in apparently valid use-cases like
This is fixed in git now, thanks!
Post by Ivan Shapovalov
or
It worked before and it still works for me.
Post by Ivan Shapovalov
Actually I understand what it does mean: systemctl asks the manager to show
unit's FragmentPath -> the manager tries to load the unit -> loading fails with
I don't see why it should fail for this reason ?
Post by Ivan Shapovalov
(BTW, it's a separate question: is this failure valid or is it a bug?)
Is this possible?
I made various unifications to the code to make it more maintainable. This
case should be fixed too. Please test it... it's easy to miss the corner cases.
Yes, that works now, thanks! Also the error messages are now a lot cleaner
and more "in line" with the rest of systemctl.
I guess that's another problem. However, the error message could be improved
by querying the manager for LoadFailed and displaying that message instead
of just saying "does not have any files on disk". I'll try to come up with a patch
for that.
Right now, I made the path for dropins shared with the code paths in
core/, but not the path for loading the main fragment. So when aliases
are involved, systemctl might do things differently than systemd.
src/core/load-fragmnet.c does something like walk the list of
directories, look for symlinks, and remember all the names. Than it
loads configuration (and dropins) based on the full set of name.
Post by Ivan Shapovalov
BTW, would it be good to have a LoadFailedErrno property or something in that line
to get an errno instead of a string? Or is it better to convert string to errno
using sd_bus_error_*() methods?
One errno would not be enough, because potentially you could have many dropins
which failed to load for some strange reason. I'd ignore that, you can always
run 'SYSTEMD_LOG_LEVEL=debug systemctl cat --root=/ ...' if you want to have
full logs of what is loaded or not loaded.
Post by Ivan Shapovalov
I'm sorry for too many questions :P
Fine with me :)

Zbyszek
Ivan Shapovalov
2014-12-19 14:08:07 UTC
Permalink
---
src/shared/install.c | 16 ----------------
src/shared/path-lookup.c | 16 ++++++++++++++++
src/shared/path-lookup.h | 4 ++++
src/systemctl/systemctl.c | 6 +-----
4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/src/shared/install.c b/src/shared/install.c
index efbe61e..2dcdfc3 100644
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -58,22 +58,6 @@ static int in_search_path(const char *path, char **search) {
return strv_contains(search, parent);
}

-static int lookup_paths_init_from_scope(LookupPaths *paths,
- UnitFileScope scope,
- const char *root_dir) {
- assert(paths);
- assert(scope >= 0);
- assert(scope < _UNIT_FILE_SCOPE_MAX);
-
- zero(*paths);
-
- return lookup_paths_init(paths,
- scope == UNIT_FILE_SYSTEM ? SYSTEMD_SYSTEM : SYSTEMD_USER,
- scope == UNIT_FILE_USER,
- root_dir,
- NULL, NULL, NULL);
-}
-
static int get_config_path(UnitFileScope scope, bool runtime, const char *root_dir, char **ret) {
char *p = NULL;
int r;
diff --git a/src/shared/path-lookup.c b/src/shared/path-lookup.c
index 8f75a8e..051f1a4 100644
--- a/src/shared/path-lookup.c
+++ b/src/shared/path-lookup.c
@@ -398,3 +398,19 @@ void lookup_paths_free(LookupPaths *p) {
p->sysvinit_path = p->sysvrcnd_path = NULL;
#endif
}
+
+int lookup_paths_init_from_scope(LookupPaths *paths,
+ UnitFileScope scope,
+ const char *root_dir) {
+ assert(paths);
+ assert(scope >= 0);
+ assert(scope < _UNIT_FILE_SCOPE_MAX);
+
+ zero(*paths);
+
+ return lookup_paths_init(paths,
+ scope == UNIT_FILE_SYSTEM ? SYSTEMD_SYSTEM : SYSTEMD_USER,
+ scope == UNIT_FILE_USER,
+ root_dir,
+ NULL, NULL, NULL);
+}
diff --git a/src/shared/path-lookup.h b/src/shared/path-lookup.h
index b8a0aac..655e454 100644
--- a/src/shared/path-lookup.h
+++ b/src/shared/path-lookup.h
@@ -22,6 +22,7 @@
***/

#include "macro.h"
+#include "install.h"

typedef struct LookupPaths {
char **unit_path;
@@ -49,5 +50,8 @@ int lookup_paths_init(LookupPaths *p,
const char *generator_early,
const char *generator_late);
void lookup_paths_free(LookupPaths *p);
+int lookup_paths_init_from_scope(LookupPaths *paths,
+ UnitFileScope scope,
+ const char *root_dir);

#define _cleanup_lookup_paths_free_ _cleanup_(lookup_paths_free)
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 4c4648f..7af111c 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -4742,11 +4742,7 @@ static int init_home_and_lookup_paths(char **user_home, char **user_runtime, Loo
return log_error_errno(ENOTDIR, "Cannot find units: $XDG_RUNTIME_DIR is not set.");
}

- r = lookup_paths_init(lp,
- arg_scope == UNIT_FILE_SYSTEM ? SYSTEMD_SYSTEM : SYSTEMD_USER,
- arg_scope == UNIT_FILE_USER,
- arg_root,
- NULL, NULL, NULL);
+ r = lookup_paths_init_from_scope(lp, arg_scope, arg_root);
if (r < 0)
return log_error_errno(r, "Failed to lookup unit lookup paths: %m");
--
2.2.0
Ivan Shapovalov
2014-12-19 14:08:08 UTC
Permalink
"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
Zbigniew Jędrzejewski-Szmek
2015-01-05 16:05:34 UTC
Permalink
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.
Post by Ivan Shapovalov
"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.
Post by Ivan Shapovalov
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.
Post by Ivan Shapovalov
+
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.
Post by Ivan Shapovalov
+ 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
Lennart Poettering
2015-02-03 22:05:02 UTC
Permalink
On Fri, 19.12.14 17:08, Ivan Shapovalov (***@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
Post by Ivan Shapovalov
"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
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2015-02-04 00:31:00 UTC
Permalink
Post by Lennart Poettering
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 /
Ivan Shapovalov
2014-12-19 14:08:10 UTC
Permalink
---
src/systemctl/systemctl.c | 63 ++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 28 deletions(-)

diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 20c367c..2a4e2a2 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -2309,6 +2309,9 @@ static int unit_find_paths(sd_bus *bus,
LookupPaths *lp,
char **fragment_path,
char ***dropin_paths) {
+
+ _cleanup_free_ char *path = NULL;
+ _cleanup_strv_free_ char **dropins = NULL;
int r;

/**
@@ -2327,8 +2330,6 @@ static int unit_find_paths(sd_bus *bus,
_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;

@@ -2375,28 +2376,17 @@ static int unit_find_paths(sd_bus *bus,
if (r < 0)
return log_error_errno(r, "Failed to get FragmentPath: %s", bus_error_message(&error, r));

- r = sd_bus_get_property_strv(
- bus,
- "org.freedesktop.systemd1",
- unit,
- "org.freedesktop.systemd1.Unit",
- "DropInPaths",
- &error,
- &dropins);
- if (r < 0)
- return log_error_errno(r, "Failed to get DropInPaths: %s", bus_error_message(&error, r));
-
- r = 0;
- if (!isempty(path)) {
- *fragment_path = path;
- path = NULL;
- r = 1;
- }
-
- if (dropin_paths && !strv_isempty(dropins)) {
- *dropin_paths = dropins;
- dropins = NULL;
- r = 1;
+ if (dropin_paths) {
+ r = sd_bus_get_property_strv(
+ bus,
+ "org.freedesktop.systemd1",
+ unit,
+ "org.freedesktop.systemd1.Unit",
+ "DropInPaths",
+ &error,
+ &dropins);
+ if (r < 0)
+ return log_error_errno(r, "Failed to get DropInPaths: %s", bus_error_message(&error, r));
}
} else {
_cleanup_set_free_ Set *names;
@@ -2409,7 +2399,7 @@ static int unit_find_paths(sd_bus *bus,
if (r < 0)
return r;

- r = unit_file_find_path(lp, unit_name, fragment_path);
+ r = unit_file_find_path(lp, unit_name, &path);
if (r < 0)
return r;

@@ -2421,14 +2411,31 @@ static int unit_find_paths(sd_bus *bus,
return log_oom();

if (!streq(template, unit_name)) {
- r = unit_file_find_path(lp, template, fragment_path);
+ r = unit_file_find_path(lp, template, &path);
if (r < 0)
return r;
}
}

- if (dropin_paths)
- r = unit_file_find_dropin_paths(lp->unit_path, NULL, names, dropin_paths);
+ if (dropin_paths) {
+ r = unit_file_find_dropin_paths(lp->unit_path, NULL, names, &dropins);
+ if (r < 0)
+ return r;
+ }
+ }
+
+ r = 0;
+
+ if (!isempty(path)) {
+ *fragment_path = path;
+ path = NULL;
+ r = 1;
+ }
+
+ if (dropin_paths && !strv_isempty(dropins)) {
+ *dropin_paths = dropins;
+ dropins = NULL;
+ r = 1;
}

if (r == 0) {
--
2.2.0
Zbigniew Jędrzejewski-Szmek
2015-01-05 16:11:15 UTC
Permalink
"two code paths".

1/4 and 4/4 are fine, but I think it's better to have them applied
adjacent to the remaing two, so I leave them for now too.

Zbyszek
Post by Ivan Shapovalov
---
src/systemctl/systemctl.c | 63 ++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 28 deletions(-)
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 20c367c..2a4e2a2 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -2309,6 +2309,9 @@ static int unit_find_paths(sd_bus *bus,
LookupPaths *lp,
char **fragment_path,
char ***dropin_paths) {
+
+ _cleanup_free_ char *path = NULL;
+ _cleanup_strv_free_ char **dropins = NULL;
int r;
/**
@@ -2327,8 +2330,6 @@ static int unit_find_paths(sd_bus *bus,
_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;
@@ -2375,28 +2376,17 @@ static int unit_find_paths(sd_bus *bus,
if (r < 0)
return log_error_errno(r, "Failed to get FragmentPath: %s", bus_error_message(&error, r));
- r = sd_bus_get_property_strv(
- bus,
- "org.freedesktop.systemd1",
- unit,
- "org.freedesktop.systemd1.Unit",
- "DropInPaths",
- &error,
- &dropins);
- if (r < 0)
- return log_error_errno(r, "Failed to get DropInPaths: %s", bus_error_message(&error, r));
-
- r = 0;
- if (!isempty(path)) {
- *fragment_path = path;
- path = NULL;
- r = 1;
- }
-
- if (dropin_paths && !strv_isempty(dropins)) {
- *dropin_paths = dropins;
- dropins = NULL;
- r = 1;
+ if (dropin_paths) {
+ r = sd_bus_get_property_strv(
+ bus,
+ "org.freedesktop.systemd1",
+ unit,
+ "org.freedesktop.systemd1.Unit",
+ "DropInPaths",
+ &error,
+ &dropins);
+ if (r < 0)
+ return log_error_errno(r, "Failed to get DropInPaths: %s", bus_error_message(&error, r));
}
} else {
_cleanup_set_free_ Set *names;
@@ -2409,7 +2399,7 @@ static int unit_find_paths(sd_bus *bus,
if (r < 0)
return r;
- r = unit_file_find_path(lp, unit_name, fragment_path);
+ r = unit_file_find_path(lp, unit_name, &path);
if (r < 0)
return r;
@@ -2421,14 +2411,31 @@ static int unit_find_paths(sd_bus *bus,
return log_oom();
if (!streq(template, unit_name)) {
- r = unit_file_find_path(lp, template, fragment_path);
+ r = unit_file_find_path(lp, template, &path);
if (r < 0)
return r;
}
}
- if (dropin_paths)
- r = unit_file_find_dropin_paths(lp->unit_path, NULL, names, dropin_paths);
+ if (dropin_paths) {
+ r = unit_file_find_dropin_paths(lp->unit_path, NULL, names, &dropins);
+ if (r < 0)
+ return r;
+ }
+ }
+
+ r = 0;
+
+ if (!isempty(path)) {
+ *fragment_path = path;
+ path = NULL;
+ r = 1;
+ }
+
+ if (dropin_paths && !strv_isempty(dropins)) {
+ *dropin_paths = dropins;
+ dropins = NULL;
+ r = 1;
}
if (r == 0) {
--
2.2.0
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Ivan Shapovalov
2014-12-19 14:08:09 UTC
Permalink
---
src/systemctl/systemctl.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 658793e..20c367c 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -4776,7 +4776,7 @@ static int init_home_and_lookup_paths(char **user_home, char **user_runtime, Loo

r = lookup_paths_init_from_scope(lp, arg_scope, arg_root);
if (r < 0)
- return log_error_errno(r, "Failed to lookup unit lookup paths: %m");
+ return log_error_errno(r, "Failed to get unit lookup paths: %m");

return 0;
}
@@ -5900,11 +5900,11 @@ static int create_edit_temp_file(const char *new_path, const char *original_path

r = tempfn_random(new_path, &t);
if (r < 0)
- return log_error_errno(r, "Failed to determine temporary filename for %s: %m", new_path);
+ return log_error_errno(r, "Failed to determine temporary filename for \"%s\": %m", new_path);

r = mkdir_parents(new_path, 0755);
if (r < 0) {
- log_error_errno(r, "Failed to create directories for %s: %m", new_path);
+ log_error_errno(r, "Failed to create directories for \"%s\": %m", new_path);
free(t);
return r;
}
@@ -5913,12 +5913,12 @@ static int create_edit_temp_file(const char *new_path, const char *original_path
if (r == -ENOENT) {
r = touch(t);
if (r < 0) {
- log_error_errno(r, "Failed to create temporary file %s: %m", t);
+ log_error_errno(r, "Failed to create temporary file \"%s\": %m", t);
free(t);
return r;
}
} else if (r < 0) {
- log_error_errno(r, "Failed to copy %s to %s: %m", original_path, t);
+ log_error_errno(r, "Failed to copy \"%s\" to \"%s\": %m", original_path, t);
free(t);
return r;
}
@@ -6026,7 +6026,7 @@ static int unit_file_create_copy(const char *unit_name,
if (!path_equal(fragment_path, tmp_new_path) && access(tmp_new_path, F_OK) == 0) {
char response;

- r = ask_char(&response, "yn", "%s already exists, are you sure to overwrite it with %s? [(y)es, (n)o] ", tmp_new_path, fragment_path);
+ r = ask_char(&response, "yn", "\"%s\" already exists, are you sure to overwrite it with \"%s\"? [(y)es, (n)o] ", tmp_new_path, fragment_path);
if (r < 0) {
free(tmp_new_path);
return r;
@@ -6040,7 +6040,7 @@ static int unit_file_create_copy(const char *unit_name,

r = create_edit_temp_file(tmp_new_path, fragment_path, &tmp_tmp_path);
if (r < 0) {
- log_error_errno(r, "Failed to create temporary file for %s: %m", tmp_new_path);
+ log_error_errno(r, "Failed to create temporary file for \"%s\": %m", tmp_new_path);
free(tmp_new_path);
return r;
}
@@ -6176,12 +6176,12 @@ static int edit(sd_bus *bus, char **args) {
assert(args);

if (!on_tty()) {
- log_error("Cannot edit units if we are not on a tty");
+ log_error("Not on a tty, refusing.");
return -EINVAL;
}

if (arg_transport != BUS_TRANSPORT_LOCAL) {
- log_error("Cannot remotely edit units");
+ log_error("Remote operation requested, refusing.");
return -EINVAL;
}

@@ -6205,12 +6205,12 @@ static int edit(sd_bus *bus, char **args) {
* It's useful if the user wants to cancel its modification
*/
if (null_or_empty_path(*tmp)) {
- log_warning("Edition of %s canceled: temporary file empty", *original);
+ log_warning("Temporary file empty, not saving.");
continue;
}
r = rename(*tmp, *original);
if (r < 0) {
- r = log_error_errno(errno, "Failed to rename %s to %s: %m", *tmp, *original);
+ r = log_error_errno(errno, "Failed to rename \"%s\" to \"%s\": %m", *tmp, *original);
goto end;
}
}
--
2.2.0
Zbigniew Jędrzejewski-Szmek
2015-01-05 16:08:39 UTC
Permalink
Post by Ivan Shapovalov
---
src/systemctl/systemctl.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 658793e..20c367c 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -4776,7 +4776,7 @@ static int init_home_and_lookup_paths(char **user_home, char **user_runtime, Loo
r = lookup_paths_init_from_scope(lp, arg_scope, arg_root);
if (r < 0)
- return log_error_errno(r, "Failed to lookup unit lookup paths: %m");
+ return log_error_errno(r, "Failed to get unit lookup paths: %m");
Maybe "query"? "get" is ugly.
Post by Ivan Shapovalov
return 0;
}
@@ -5900,11 +5900,11 @@ static int create_edit_temp_file(const char *new_path, const char *original_path
r = tempfn_random(new_path, &t);
if (r < 0)
- return log_error_errno(r, "Failed to determine temporary filename for %s: %m", new_path);
+ return log_error_errno(r, "Failed to determine temporary filename for \"%s\": %m", new_path);
r = mkdir_parents(new_path, 0755);
if (r < 0) {
- log_error_errno(r, "Failed to create directories for %s: %m", new_path);
+ log_error_errno(r, "Failed to create directories for \"%s\": %m", new_path);
free(t);
return r;
}
@@ -5913,12 +5913,12 @@ static int create_edit_temp_file(const char *new_path, const char *original_path
if (r == -ENOENT) {
r = touch(t);
if (r < 0) {
- log_error_errno(r, "Failed to create temporary file %s: %m", t);
+ log_error_errno(r, "Failed to create temporary file \"%s\": %m", t);
free(t);
return r;
}
} else if (r < 0) {
- log_error_errno(r, "Failed to copy %s to %s: %m", original_path, t);
+ log_error_errno(r, "Failed to copy \"%s\" to \"%s\": %m", original_path, t);
free(t);
return r;
}
@@ -6026,7 +6026,7 @@ static int unit_file_create_copy(const char *unit_name,
if (!path_equal(fragment_path, tmp_new_path) && access(tmp_new_path, F_OK) == 0) {
char response;
- r = ask_char(&response, "yn", "%s already exists, are you sure to overwrite it with %s? [(y)es, (n)o] ", tmp_new_path, fragment_path);
+ r = ask_char(&response, "yn", "\"%s\" already exists, are you sure to overwrite it with \"%s\"? [(y)es, (n)o] ", tmp_new_path, fragment_path);
This is not gramatically correct. I think
"\"%s\" already exists. Overwrite with \"%s\"?..." would be better.
Post by Ivan Shapovalov
if (r < 0) {
free(tmp_new_path);
return r;
@@ -6040,7 +6040,7 @@ static int unit_file_create_copy(const char *unit_name,
r = create_edit_temp_file(tmp_new_path, fragment_path, &tmp_tmp_path);
if (r < 0) {
- log_error_errno(r, "Failed to create temporary file for %s: %m", tmp_new_path);
+ log_error_errno(r, "Failed to create temporary file for \"%s\": %m", tmp_new_path);
free(tmp_new_path);
return r;
}
@@ -6176,12 +6176,12 @@ static int edit(sd_bus *bus, char **args) {
assert(args);
if (!on_tty()) {
- log_error("Cannot edit units if we are not on a tty");
+ log_error("Not on a tty, refusing.");
Why? Replacing a nice specific error message with a generic one seems to
be a step backwards.
Post by Ivan Shapovalov
return -EINVAL;
}
if (arg_transport != BUS_TRANSPORT_LOCAL) {
- log_error("Cannot remotely edit units");
+ log_error("Remote operation requested, refusing.");
And the same here.
Post by Ivan Shapovalov
return -EINVAL;
}
@@ -6205,12 +6205,12 @@ static int edit(sd_bus *bus, char **args) {
* It's useful if the user wants to cancel its modification
*/
if (null_or_empty_path(*tmp)) {
- log_warning("Edition of %s canceled: temporary file empty", *original);
+ log_warning("Temporary file empty, not saving.");
And here.
Post by Ivan Shapovalov
continue;
}
r = rename(*tmp, *original);
if (r < 0) {
- r = log_error_errno(errno, "Failed to rename %s to %s: %m", *tmp, *original);
+ r = log_error_errno(errno, "Failed to rename \"%s\" to \"%s\": %m", *tmp, *original);
goto end;
}
}
Zbyszek
Ivan Shapovalov
2015-02-04 00:35:47 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
---
src/systemctl/systemctl.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 658793e..20c367c 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -4776,7 +4776,7 @@ static int init_home_and_lookup_paths(char **user_home, char **user_runtime, Loo
r = lookup_paths_init_from_scope(lp, arg_scope, arg_root);
if (r < 0)
- return log_error_errno(r, "Failed to lookup unit lookup paths: %m");
+ return log_error_errno(r, "Failed to get unit lookup paths: %m");
Maybe "query"? "get" is ugly.
OK.
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
return 0;
}
@@ -5900,11 +5900,11 @@ static int create_edit_temp_file(const char *new_path, const char *original_path
r = tempfn_random(new_path, &t);
if (r < 0)
- return log_error_errno(r, "Failed to determine temporary filename for %s: %m", new_path);
+ return log_error_errno(r, "Failed to determine temporary filename for \"%s\": %m", new_path);
r = mkdir_parents(new_path, 0755);
if (r < 0) {
- log_error_errno(r, "Failed to create directories for %s: %m", new_path);
+ log_error_errno(r, "Failed to create directories for \"%s\": %m", new_path);
free(t);
return r;
}
@@ -5913,12 +5913,12 @@ static int create_edit_temp_file(const char *new_path, const char *original_path
if (r == -ENOENT) {
r = touch(t);
if (r < 0) {
- log_error_errno(r, "Failed to create temporary file %s: %m", t);
+ log_error_errno(r, "Failed to create temporary file \"%s\": %m", t);
free(t);
return r;
}
} else if (r < 0) {
- log_error_errno(r, "Failed to copy %s to %s: %m", original_path, t);
+ log_error_errno(r, "Failed to copy \"%s\" to \"%s\": %m", original_path, t);
free(t);
return r;
}
@@ -6026,7 +6026,7 @@ static int unit_file_create_copy(const char *unit_name,
if (!path_equal(fragment_path, tmp_new_path) && access(tmp_new_path, F_OK) == 0) {
char response;
- r = ask_char(&response, "yn", "%s already exists, are you sure to overwrite it with %s? [(y)es, (n)o] ", tmp_new_path, fragment_path);
+ r = ask_char(&response, "yn", "\"%s\" already exists, are you sure to overwrite it with \"%s\"? [(y)es, (n)o] ", tmp_new_path, fragment_path);
This is not gramatically correct. I think
"\"%s\" already exists. Overwrite with \"%s\"?..." would be better.
OK.
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
if (r < 0) {
free(tmp_new_path);
return r;
@@ -6040,7 +6040,7 @@ static int unit_file_create_copy(const char *unit_name,
r = create_edit_temp_file(tmp_new_path, fragment_path, &tmp_tmp_path);
if (r < 0) {
- log_error_errno(r, "Failed to create temporary file for %s: %m", tmp_new_path);
+ log_error_errno(r, "Failed to create temporary file for \"%s\": %m", tmp_new_path);
free(tmp_new_path);
return r;
}
@@ -6176,12 +6176,12 @@ static int edit(sd_bus *bus, char **args) {
assert(args);
if (!on_tty()) {
- log_error("Cannot edit units if we are not on a tty");
+ log_error("Not on a tty, refusing.");
Why? Replacing a nice specific error message with a generic one seems to
be a step backwards.
I've tried to follow the existing pattern of most error messages in
systemd: "Condition, action + -ing."

I'll reword again to make messages more specific... unless you tell me
to stop bikeshedding and leave it as is ;)
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
return -EINVAL;
}
if (arg_transport != BUS_TRANSPORT_LOCAL) {
- log_error("Cannot remotely edit units");
+ log_error("Remote operation requested, refusing.");
And the same here.
Post by Ivan Shapovalov
return -EINVAL;
}
@@ -6205,12 +6205,12 @@ static int edit(sd_bus *bus, char **args) {
* It's useful if the user wants to cancel its modification
*/
if (null_or_empty_path(*tmp)) {
- log_warning("Edition of %s canceled: temporary file empty", *original);
+ log_warning("Temporary file empty, not saving.");
And here.
Here? They are equally specific; "editing cancelled" == "file not
saved".
--
Ivan Shapovalov / intelfx /
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
continue;
}
r = rename(*tmp, *original);
if (r < 0) {
- r = log_error_errno(errno, "Failed to rename %s to %s: %m", *tmp, *original);
+ r = log_error_errno(errno, "Failed to rename \"%s\" to \"%s\": %m", *tmp, *original);
goto end;
}
}
Zbyszek
Zbigniew Jędrzejewski-Szmek
2015-02-04 01:03:58 UTC
Permalink
Post by Ivan Shapovalov
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
---
src/systemctl/systemctl.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 658793e..20c367c 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -4776,7 +4776,7 @@ static int init_home_and_lookup_paths(char **user_home, char **user_runtime, Loo
r = lookup_paths_init_from_scope(lp, arg_scope, arg_root);
if (r < 0)
- return log_error_errno(r, "Failed to lookup unit lookup paths: %m");
+ return log_error_errno(r, "Failed to get unit lookup paths: %m");
Maybe "query"? "get" is ugly.
OK.
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
return 0;
}
@@ -5900,11 +5900,11 @@ static int create_edit_temp_file(const char *new_path, const char *original_path
r = tempfn_random(new_path, &t);
if (r < 0)
- return log_error_errno(r, "Failed to determine temporary filename for %s: %m", new_path);
+ return log_error_errno(r, "Failed to determine temporary filename for \"%s\": %m", new_path);
r = mkdir_parents(new_path, 0755);
if (r < 0) {
- log_error_errno(r, "Failed to create directories for %s: %m", new_path);
+ log_error_errno(r, "Failed to create directories for \"%s\": %m", new_path);
free(t);
return r;
}
@@ -5913,12 +5913,12 @@ static int create_edit_temp_file(const char *new_path, const char *original_path
if (r == -ENOENT) {
r = touch(t);
if (r < 0) {
- log_error_errno(r, "Failed to create temporary file %s: %m", t);
+ log_error_errno(r, "Failed to create temporary file \"%s\": %m", t);
free(t);
return r;
}
} else if (r < 0) {
- log_error_errno(r, "Failed to copy %s to %s: %m", original_path, t);
+ log_error_errno(r, "Failed to copy \"%s\" to \"%s\": %m", original_path, t);
free(t);
return r;
}
@@ -6026,7 +6026,7 @@ static int unit_file_create_copy(const char *unit_name,
if (!path_equal(fragment_path, tmp_new_path) && access(tmp_new_path, F_OK) == 0) {
char response;
- r = ask_char(&response, "yn", "%s already exists, are you sure to overwrite it with %s? [(y)es, (n)o] ", tmp_new_path, fragment_path);
+ r = ask_char(&response, "yn", "\"%s\" already exists, are you sure to overwrite it with \"%s\"? [(y)es, (n)o] ", tmp_new_path, fragment_path);
This is not gramatically correct. I think
"\"%s\" already exists. Overwrite with \"%s\"?..." would be better.
OK.
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
if (r < 0) {
free(tmp_new_path);
return r;
@@ -6040,7 +6040,7 @@ static int unit_file_create_copy(const char *unit_name,
r = create_edit_temp_file(tmp_new_path, fragment_path, &tmp_tmp_path);
if (r < 0) {
- log_error_errno(r, "Failed to create temporary file for %s: %m", tmp_new_path);
+ log_error_errno(r, "Failed to create temporary file for \"%s\": %m", tmp_new_path);
free(tmp_new_path);
return r;
}
@@ -6176,12 +6176,12 @@ static int edit(sd_bus *bus, char **args) {
assert(args);
if (!on_tty()) {
- log_error("Cannot edit units if we are not on a tty");
+ log_error("Not on a tty, refusing.");
Why? Replacing a nice specific error message with a generic one seems to
be a step backwards.
I've tried to follow the existing pattern of most error messages in
systemd: "Condition, action + -ing."
I'll reword again to make messages more specific... unless you tell me
to stop bikeshedding and leave it as is ;)
Yeah, I think that that those messages are OK.
Post by Ivan Shapovalov
Post by Zbigniew Jędrzejewski-Szmek
Post by Ivan Shapovalov
- log_warning("Edition of %s canceled: temporary file empty", *original);
+ log_warning("Temporary file empty, not saving.");
And here.
Here? They are equally specific; "editing cancelled" == "file not
saved".
The original one gives a hint of what the overall effect for the action is.
With you replacemnt, things are less explicit.

systemctl edit is something that may be used by relatively newbie
administrators, so some hand-holding is more useful than in other parts
of systemd.

Zbyszek
Zbigniew Jędrzejewski-Szmek
2015-02-05 01:26:53 UTC
Permalink
Looks good. Pushed all four.

Zbyszek
- report actual load error for units which could not be loaded
- make unit_find_paths() report all kinds of errors it encounters
(for consistency)
- consistently handle not-found errors in cat() and edit()
---
src/systemctl/systemctl.c | 54 +++++++++++++++++++++++++++++++++++------------
1 file changed, 40 insertions(+), 14 deletions(-)
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 462f7fd..083b618 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -2309,9 +2309,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)
@@ -2320,6 +2323,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: %s", unit_name, unit_load_error_message);
+ return 0;
+ }
+
r = sd_bus_get_property_string(
bus,
"org.freedesktop.systemd1",
@@ -2387,6 +2415,9 @@ 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 %s.", unit_name);
+
return r;
}
@@ -4588,10 +4619,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);
- continue;
- }
+ else if (r == 0)
+ return -ENOENT;
if (first)
first = false;
@@ -5940,9 +5969,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)
+ return -ENOENT;
+ 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 %s.", *name);
+ return -ENOENT;
+ }
if (arg_full)
r = unit_file_create_copy(*name, path, user_home, user_runtime, &new_path, &tmp_path);
@@ -5981,19 +6014,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.2
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Ivan Shapovalov
2014-12-25 21:27:16 UTC
Permalink
Post by Ivan Shapovalov
---
src/shared/install.c | 16 ----------------
src/shared/path-lookup.c | 16 ++++++++++++++++
src/shared/path-lookup.h | 4 ++++
src/systemctl/systemctl.c | 6 +-----
4 files changed, 21 insertions(+), 21 deletions(-)
[...]
Ping?
--
Ivan Shapovalov / intelfx /
Ivan Shapovalov
2015-01-02 23:10:18 UTC
Permalink
Post by Ivan Shapovalov
---
src/shared/install.c | 16 ----------------
src/shared/path-lookup.c | 16 ++++++++++++++++
src/shared/path-lookup.h | 4 ++++
src/systemctl/systemctl.c | 6 +-----
4 files changed, 21 insertions(+), 21 deletions(-)
[...]
Ping?
Ping? :-)
--
Ivan Shapovalov / intelfx /
Zbigniew Jędrzejewski-Szmek
2015-01-03 03:23:08 UTC
Permalink
Post by Ivan Shapovalov
Post by Ivan Shapovalov
---
src/shared/install.c | 16 ----------------
src/shared/path-lookup.c | 16 ++++++++++++++++
src/shared/path-lookup.h | 4 ++++
src/systemctl/systemctl.c | 6 +-----
4 files changed, 21 insertions(+), 21 deletions(-)
[...]
Ping?
Ping? :-)
I'll review them this weekend if nobody beats me to it.

Zbyszek
Zbigniew Jędrzejewski-Szmek
2015-01-05 16:13:47 UTC
Permalink
Post by Ivan Shapovalov
---
src/shared/install.c | 16 ----------------
src/shared/path-lookup.c | 16 ++++++++++++++++
src/shared/path-lookup.h | 4 ++++
src/systemctl/systemctl.c | 6 +-----
4 files changed, 21 insertions(+), 21 deletions(-)
Actually 1/4 is kind of independent. Applied.

Zbyszek
Post by Ivan Shapovalov
diff --git a/src/shared/install.c b/src/shared/install.c
index efbe61e..2dcdfc3 100644
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -58,22 +58,6 @@ static int in_search_path(const char *path, char **search) {
return strv_contains(search, parent);
}
-static int lookup_paths_init_from_scope(LookupPaths *paths,
- UnitFileScope scope,
- const char *root_dir) {
- assert(paths);
- assert(scope >= 0);
- assert(scope < _UNIT_FILE_SCOPE_MAX);
-
- zero(*paths);
-
- return lookup_paths_init(paths,
- scope == UNIT_FILE_SYSTEM ? SYSTEMD_SYSTEM : SYSTEMD_USER,
- scope == UNIT_FILE_USER,
- root_dir,
- NULL, NULL, NULL);
-}
-
static int get_config_path(UnitFileScope scope, bool runtime, const char *root_dir, char **ret) {
char *p = NULL;
int r;
diff --git a/src/shared/path-lookup.c b/src/shared/path-lookup.c
index 8f75a8e..051f1a4 100644
--- a/src/shared/path-lookup.c
+++ b/src/shared/path-lookup.c
@@ -398,3 +398,19 @@ void lookup_paths_free(LookupPaths *p) {
p->sysvinit_path = p->sysvrcnd_path = NULL;
#endif
}
+
+int lookup_paths_init_from_scope(LookupPaths *paths,
+ UnitFileScope scope,
+ const char *root_dir) {
+ assert(paths);
+ assert(scope >= 0);
+ assert(scope < _UNIT_FILE_SCOPE_MAX);
+
+ zero(*paths);
+
+ return lookup_paths_init(paths,
+ scope == UNIT_FILE_SYSTEM ? SYSTEMD_SYSTEM : SYSTEMD_USER,
+ scope == UNIT_FILE_USER,
+ root_dir,
+ NULL, NULL, NULL);
+}
diff --git a/src/shared/path-lookup.h b/src/shared/path-lookup.h
index b8a0aac..655e454 100644
--- a/src/shared/path-lookup.h
+++ b/src/shared/path-lookup.h
@@ -22,6 +22,7 @@
***/
#include "macro.h"
+#include "install.h"
typedef struct LookupPaths {
char **unit_path;
@@ -49,5 +50,8 @@ int lookup_paths_init(LookupPaths *p,
const char *generator_early,
const char *generator_late);
void lookup_paths_free(LookupPaths *p);
+int lookup_paths_init_from_scope(LookupPaths *paths,
+ UnitFileScope scope,
+ const char *root_dir);
#define _cleanup_lookup_paths_free_ _cleanup_(lookup_paths_free)
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 4c4648f..7af111c 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -4742,11 +4742,7 @@ static int init_home_and_lookup_paths(char **user_home, char **user_runtime, Loo
return log_error_errno(ENOTDIR, "Cannot find units: $XDG_RUNTIME_DIR is not set.");
}
- r = lookup_paths_init(lp,
- arg_scope == UNIT_FILE_SYSTEM ? SYSTEMD_SYSTEM : SYSTEMD_USER,
- arg_scope == UNIT_FILE_USER,
- arg_root,
- NULL, NULL, NULL);
+ r = lookup_paths_init_from_scope(lp, arg_scope, arg_root);
if (r < 0)
return log_error_errno(r, "Failed to lookup unit lookup paths: %m");
--
2.2.0
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Continue reading on narkive:
Loading...