[systemd-devel] [PATCH v3] systemctl: add edit verb

[Lennart Poettering]

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.

> +
> +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?

Lennart

-- 
Lennart Poettering, Red Hat