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

[Ronny Chevalier]

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.
>
>> +
>> +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.
Oh and about this one, since I saw you mentioning it a couple of times
in other patches sent to the ML, maybe you should add this to the
CODING_STYLE ? I don't see any mention of this in it currently.

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