[systemd-devel] [PATCH] systemctl: introduce --now for enable, disable and mask

[Lennart Poettering]

On Thu, 14.05.15 14:26, jsynacek at redhat.com (jsynacek at redhat.com) wrote:

>  const char *unit_file_preset_mode_to_string(UnitFilePresetMode m) _const_;
>  UnitFilePresetMode unit_file_preset_mode_from_string(const char *s) _pure_;
> +
> +
> +int add_file_change(UnitFileChange **changes, unsigned *n_changes,
>  UnitFileChangeType type, const char *path, const char *source);

No double newlines please.

More importantly though, if you export this function it should really
carry some useful namespace prefix, and not begin with the verb. 

Given that we already have unit_file_changes_free() as public call, it
should probably be called unit_file_changes_add() and be placed next
to it in the header and .c file.

Having good names for static, internal calls is not as important as
for non-static, exported ones, hence the need for the rename.

> +        if (arg_now && n_changes > 0 && STR_IN_SET(args[0], "enable", "disable", "mask")) {
> +                _cleanup_strv_free_ char **new_args = NULL;
> +                unsigned i;
> +
> +                r = strv_push(&new_args, streq(args[0], "enable") ? strdup("start") : strdup("stop"));
> +                if (r < 0)
> +                        goto finish;

This will crash on OOM.

strv_extend() internally does strdup() + strv_push() and checks for
OOM, hence that sounds like the better option.

That said, both strv_extend() and strv_push() internally reallocate
the string array. That's fine for small arrays, or when you don't know
how many entries to expect, but in this case you really do know, you
get it passed back in n_changes after all. Hence, please make use of
this, allocate a temporary array (maybe with newa()) then add the
entries to it, use and forget it.

> +
> +                for (i = 0; i < n_changes; i++) {
> +                        r = strv_push(&new_args,
> strdup(strrchr(changes[i].path, '/') + 1));

The strrchr appears to be something where useing basename() is the
better option...

Lennart

-- 
Lennart Poettering, Red Hat