[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:

Three more suggestions after thinking about this a bit more...

> +static int unit_file_drop_in(const char *unit_name, const char *config_home, char **new_path) {
> +        char *tmp_path;
> +        int r;
> +
> +        assert(unit_name);
> +        assert(new_path);
> +
> +        switch (arg_scope) {
> +                case UNIT_FILE_SYSTEM:
> +                        tmp_path = strjoin(SYSTEM_CONFIG_UNIT_PATH, "/", unit_name, ".d/amendments.conf", NULL);
> +                        break;
> +                case UNIT_FILE_GLOBAL:
> +                        tmp_path = strjoin(USER_CONFIG_UNIT_PATH, "/", unit_name, ".d/amendments.conf", NULL);
> +                        break;
> +                case UNIT_FILE_USER:
> +                        assert(config_home);
> +                        tmp_path = strjoin(config_home, "/", unit_name, ".d/amendments.conf", NULL);
> +                        break;
> +                default:
> +                        assert_not_reached("Invalid scope");
> +        }
> +        if (!tmp_path)
> +                return log_oom();
> +
> +        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;
> +        }
> +
> +        *new_path = tmp_path;
> +
> +        return 0;

I think we should really take some inspiration here from how git
prepares the editor when asking for commit messages: adding a really
useful commented help text to the end of the new file to edit would be
really awesome. More specifically, I think we should include an output
equivalent to "systemctl cat"'s add the end, commented and prefixed
with some clear marker that we can then use to remove the bits again
before saving things.

> +        if (arg_scope == UNIT_FILE_USER
> +            || startswith(fragment_path, SYSTEM_CONFIG_UNIT_PATH)
> +            || startswith(fragment_path, USER_CONFIG_UNIT_PATH)) {

Please use path_startswith() for this.

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

Hmm, when precisely is this actually triggered? I mean, if the file
already exists in /etc we should just edit it there...

Lennart

-- 
Lennart Poettering, Red Hat