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

[Ronny Chevalier]

2014-10-21 11:09 GMT+02:00 Lennart Poettering <lennart at poettering.net>:
> 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.
Yeah It seems nice.

Do you prefer that I add this in the correction of this patch, or with
follow-up patches ?

>
>> +        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.
Oh I didn't know this one, thanks.

>
>> +        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...
There is a specific case when this can happen.

Imagine you have a unit foobar with
FragmentPath=/usr/lib/systemd/system/foobar.service, and you added
manually /etc/systemd/system/foobar.service without doing
daemon-reload. When avoid_bus() == false, we will get the /usr/***
path, so we need to be sure that the user don't want to overwrite the
one in /etc/***.

We can avoid this by doing daemon-reload before editing, but I'm not
sure this is wanted.

>
> Lennart
>
> --
> Lennart Poettering, Red Hat

Thanks for the review!