[systemd-devel] [PATCH] fstab-generator: add x-systemd.requires and x-systemd.requires-mounts-for

[Lennart Poettering]

On Fri, 15.05.15 12:41, Karel Zak (kzak at redhat.com) wrote:

> +static int write_requires_after(FILE *f, const char *opts) {
> +        _cleanup_free_ char *arg = NULL, *unit = NULL;
> +        int r;
> +
> +        assert(f);
> +        assert(opts);
> +
> +        r = fstab_filter_options(opts, "x-systemd.requires\0", NULL, &arg, NULL);
> +        if (r < 0)
> +                return log_warning_errno(r, "Failed to parse options: %m");
> +        if (r == 0)
> +                return 0;

Shouldn't this also use the new fstab_extract_values() call? I mean
people might want to specify multiple of these, too?

>  
> +int fstab_extract_values(const char *opts, const char *name, char ***values)
> +{
> +        _cleanup_strv_free_ char **optsv = NULL, **res = NULL;
> +        char **s;


Minor nitpick: we usually place the opening brackets on the same line as the
function name:

int fstab_extract_values(...) {

rather than:

int fstab_extract_values(...) 
{

> +
> +        assert(opts);
> +        assert(name);
> +        assert(values);
> +
> +        optsv = strv_split(opts, ",");
> +        if (!optsv)
> +                return -ENOMEM;
> +
> +        STRV_FOREACH(s, optsv) {
> +                char *arg;
> +
> +                arg = startswith(*s, name);
> +                if (!arg || *arg != '=')
> +                        continue;
> +
> +                arg = strdup(arg + 1);
> +                if (!arg)
> +                        return -ENOMEM;
> +                if (!res)
> +                        res = strv_new(arg, NULL);
> +                else
> +                        strv_push(&res, arg);

This misses an OOM check.

Also, if res is NULL strv_push() will already create a new array
implicitly for you. For most strv_xyz() calls we consider a NULL strv
array identical to an empty array... 

This basically means you can drop the explicit strv_new(), and just
leave strv_push() in place.

That said, you can also drop the strdup(arg + 1) before, and simply
make use of strv_extend() which internally does the strdup() and then
uses strv_push() to add it to the list... 

Lennart

-- 
Lennart Poettering, Red Hat