[systemd-devel] [PATCH v3] systemctl: add edit verb
Ronny Chevalier
chevalier.ronny at gmail.com
Mon Oct 20 18:01:17 PDT 2014
2014-10-21 0:51 GMT+02:00 Lennart Poettering <lennart at poettering.net>:
> On Sat, 18.10.14 18:30, Ronny Chevalier (chevalier.ronny at gmail.com) wrote:
>
> Looks pretty good. A few comments.
>
>> +
>> +static int unit_file_copy_if_needed(const char *unit_name, const char *fragment_path, char **new_path) {
>> + char *tmp_path;
>> + int r;
>> +
>> + assert(fragment_path);
>> + assert(unit_name);
>> + assert(new_path);
>> +
>> + /* If it's a unit for the --user scope there is no need to copy it, it's already in the right directory.
>> + * Same if this is --system/--global scope and the file is in {SYSTEM,USER}_CONFIG_UNIT_PATH
>> + */
>> + if (arg_scope == UNIT_FILE_USER
>> + || startswith(fragment_path, SYSTEM_CONFIG_UNIT_PATH)
>> + || startswith(fragment_path, USER_CONFIG_UNIT_PATH)) {
>> + *new_path = strdup(fragment_path);
>> + if (!*new_path)
>> + return log_oom();
>> + return 0;
>> + }
>> +
>> + switch (arg_scope) {
>> + case UNIT_FILE_SYSTEM:
>> + tmp_path = strjoin(SYSTEM_CONFIG_UNIT_PATH, "/", unit_name, NULL);
>> + break;
>> + case UNIT_FILE_GLOBAL:
>> + tmp_path = strjoin(USER_CONFIG_UNIT_PATH, "/", unit_name, NULL);
>> + break;
>> + default:
>> + assert_not_reached("Invalid scope");
>> + }
>> + if (!tmp_path)
>> + return log_oom();
>> +
>> + if (access(tmp_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_path, fragment_path);
>> + if (r < 0) {
>> + free(tmp_path);
>> + return r;
>> + }
>> + if (response != 'y') {
>> + log_warning("%s ignored", unit_name);
>> + free(tmp_path);
>> + return -1;
>> + }
>> + }
>> +
>> + r = mkdir_parents(tmp_path, 0755);
>> + if (r < 0) {
>> + log_error("Failed to create directories for %s: %s", tmp_path, strerror(-r));
>> + free(tmp_path);
>> + return r;
>> + }
>> +
>> + r = copy_file(fragment_path, tmp_path, 0, 0644);
>> + if (r < 0) {
>> + log_error("Failed to copy %s to %s: %s", fragment_path, tmp_path, strerror(-r));
>> + free(tmp_path);
>> + return r;
>> + }
>> +
>> + *new_path = tmp_path;
>> +
>> + return 0;
>> +}
>> +
>> +static int get_editors(char ***editors) {
>> + char **tmp_editors = strv_new("nano", "vim", "vi", NULL);
>
> Please avoid calling functions and declaring variables in one line.
>
> Also, there's an OOM check missing for this line.
>
>> + char *editor;
>> +
>> + /* SYSTEMD_EDITOR takes precedence over EDITOR which takes precedence over VISUAL
>> + * If neither SYSTEMD_EDITOR nor EDITOR nor VISUAL are present,
>> + * we try to execute well known editors
>> + */
>> + editor = getenv("SYSTEMD_EDITOR");
>> + if (!editor)
>> + editor = getenv("EDITOR");
>> + if (!editor)
>> + editor = getenv("VISUAL");
>> +
>> + if (editor) {
>> + int r;
>> +
>> + editor = strdup(editor);
>> + if (!editor)
>> + return log_oom();
>> +
>> + r = strv_consume_prepend(&tmp_editors, editor);
>> + if (r < 0)
>> + return log_oom();
>> + }
>> +
>> + *editors = tmp_editors;
>> +
>> + return 0;
>> +}
>
> Hmm, I don't like this bit. Instead of figuring out the editor in one
> step, and then executing it, I'd really prefer if we'd try to execute
> the editors one-by-one until we find one that works. When we invoke
> the pager we follow a similar logic.
Actually that's what I do. I fill a strv of editors and try to execute
each one of them, until one works. But we fail if the error is
different than "No such file or directory".
This function (get_editors) is used in run_editor, where I do a
STRV_FOREACH in the child to try to execute every editor, with the one
comming from SYSTEMD_EDITOR or EDITOR or VISUAL, first.
Or maybe, you meant that if the one coming from SYSTEMD_EDITOR failed,
we should try to execute EDITOR, then VISUAL, too? Because I think we
should do that, but with the current code we don't.
>
>> +
>> +static int run_editor(char **paths) {
>> + pid_t pid;
>> + siginfo_t status;
>> + int r;
>> +
>> + assert(paths);
>> +
>> + pid = fork();
>> + if (pid < 0) {
>> + log_error("Failed to fork: %m");
>> + return -errno;
>> + }
>> +
>> + if (pid == 0) {
>> + _cleanup_strv_free_ char **editors = NULL;
>> + char *editor;
>> + char **p;
>> +
>> + r = get_editors(&editors);
>> + if (r < 0) {
>> + _exit(EXIT_FAILURE);
>> + }
>
> Nitpick: please no {} for single-line if blocks.
>
>> +
>> + STRV_FOREACH(p, editors) {
>> + _cleanup_strv_free_ char **args = NULL;
>> +
>> + editor = strdup(*p);
>> + if (!editor) {
>> + log_oom();
>> + _exit(EXIT_FAILURE);
>> + }
>> +
>> + args = strv_copy(paths);
>> + if (!args) {
>> + log_oom();
>> + _exit(EXIT_FAILURE);
>> + }
>> +
>> + r = strv_consume_prepend(&args, editor);
>> + if (r < 0) {
>> + log_oom();
>> + _exit(EXIT_FAILURE);
>> + }
>> +
>> + execvp(editor, args);
>> + /* We do not fail if the editor doesn't exist
>> + * because we want to try each one of them before
>> + * failing.
>> + */
>> + if (errno != ENOENT) {
>> + log_error("Failed to execute %s: %m", editor);
>> + _exit(EXIT_FAILURE);
>> + }
>> + }
>> +
>> + log_error("Cannot edit unit(s): No editor available. Please set either SYSTEMD_EDITOR or EDITOR or VISUAL environment variable");
>> + _exit(EXIT_FAILURE);
>> + }
>> +
>> + r = wait_for_terminate(pid, &status);
>> + if (r < 0) {
>> + log_error("Failed to wait for child: %s", strerror(-r));
>> + return r;
>> + }
>> +
>> + return WIFEXITED(status) ? WEXITSTATUS(status) : -1;
>
> This should really fail with an error message and a real non-success
> error code.
>
>> +
>> + if (arg_transport != BUS_TRANSPORT_LOCAL) {
>> + log_error("Cannot remotely edit units");
>> + return -EINVAL;
>> + }
>> +
>> + if (arg_runtime) {
>> + log_error("Cannot edit runtime units");
>> + return -EINVAL;
>> + }
>
> Hmm, why not support this? And imply --runtime or so?
You are right, I forgot to ask a question about this one, sorry.
If there is a unit in /etc/systemd/system/ and someone wants to edit
this unit temporarily it will run systemctl --runtime myunit. The
problem is the unit will be copied in the /run/ directory but this
directory do not have precedence over /etc/systemd/, so when this unit
will be restarted, it won't take into account the one in /run/, right?
(My understanding of this come from src/shared/path-lookup.c)
Furthermore, if I recall correctly the FragmentPath of a runtime unit is empty?
So, if I'm wrong and systemctl understands that when we do restart
with --runtime it means to execute the one from /run/, well I can
certainly add support for this.
If I'm a right, I do not see the point because it will not be easy to
understand for a user that it can temporarily edit a unit if it's in
/usr/lib/systemd/ but not in /etc/.
>
> Lennart
>
> --
> Lennart Poettering, Red Hat
More information about the systemd-devel
mailing list