[systemd-devel] [PATCH v9] tmpfiles, man: Add xattr support to tmpfiles

[Lennart Poettering]

On Wed, 03.12.14 15:33, Maciej Wereski (m.wereski at partner.samsung.com) wrote:

>  
> +static int get_xattrs_from_arg(Item *i) {
> +        char *xattr, *name, *value;
> +        const char *p;
> +        int r;

Please declare name and value in the inner scope, no need to define
them broader than necessary.

> +
> +        assert(i);
> +
> +        if (!i->argument) {
> +                log_error("%s: Argument can't be empty!", i->path);
> +                return -EBADMSG;
> +        }
> +        p = i->argument;
> +
> +        while ((r = unquote_first_word(&p, &xattr, false)) > 0) {
> +                _cleanup_free_ char *tmp = NULL;
> +                r = split_pair(xattr, "=", &name, &value);
> +                if (r < 0) {
> +                        log_warning("Illegal xattr found: \"%s\" - ignoring.", xattr);
> +                        free(xattr);
> +                        continue;
> +                }
> +                free(xattr);
> +                if (streq(name, "") || streq(value, "")) {
> +                        log_warning("Malformed xattr found: \"%s=%s\" - ignoring.", name, value);
> +                        free(name);
> +                        free(value);
> +                        continue;
> +                }
> +                tmp = unquote(value, "\"");
> +                if (!tmp)
> +                        return log_oom();

If this OOM path is hit, you don't free name nor value. I'd probably
declare them in the inner scope with _cleanup_free_ so that they are
automatically freed (which of course means you need to override the
vars with NULL explicitly if you want to disable the freeing.

> +                free(value);
> +                value = cunescape(tmp);
> +                if (!value) {
> +                        free(name);
> +                        return log_oom();
> +                }
> +                if (strv_consume(&i->xattrs, name) < 0) {
> +                        free(value);
> +                        return log_oom();
> +                }
> +                if (strv_consume(&i->xattrs, value) < 0){
> +                        free(value);
> +                        return log_oom();
> +                }

strv_consume() actually frees the passed string on failure anyway,
that's why it is call "consume"... The code above would hence free
"value" twice on failure. Use "strv_push()" if you don't want the call
to free the string on failure...

Hmm, I think it would be a good idea to introduce a new
strv_push_pair() and strv_consume_pair() for cases like this, which
add two elements at once. I added this now for you, this should make
the code a bit simple for you.

> +        }
> +
> +        r = strv_length(i->xattrs) % 2 != 0 ? -EBADMSG : 0;

This check appears unnecessary if the inner error checks work
correctly. (Also it should really print a message on failure here...)

Otherwise looks good!

Lennart

-- 
Lennart Poettering, Red Hat