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

[Zbigniew Jędrzejewski-Szmek]

On Wed, Oct 22, 2014 at 12:29:15AM +0200, Ronny Chevalier wrote:
> 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.
Hey,

this one will be on you, once you get the commit privileges ;)

Zbyszek