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

[Lennart Poettering]

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

>  
> +#ifdef HAVE_XATTR
> +static int get_xattrs_from_arg(Item *i){
> +        _cleanup_free_ char *xattr = NULL;
> +        _cleanup_strv_free_ char **tmp = NULL;
> +        char *p;
> +        unsigned n, len, strv_len;
> +
> +        assert(i);
> +        if (i->type != SET_XATTR)
> +                return 0;
> +
> +        if (!i->argument) {
> +                log_error("%s: Argument can't be empty!", i->path);
> +                return -EBADMSG;
> +        }
> +        xattr = new0(char, strlen(i->argument)+1);
> +        if (!xattr)
> +                return log_oom();
> +
> +        tmp = strv_split(i->argument, WHITESPACE);
> +        if (!tmp)
> +                return log_oom();
> +
> +        strv_len = strv_length(tmp);
> +        for (n = 0; n < strv_len; ++n) {

Sounds like a job for the STRV_FOREACH() macro. Since you don't actually
need the strv as strv here it sounds like you actually really want to
use FOREACH_WORD_QUOTED() for this, which will also do the unquoting for
you.

> +                len = strlen(tmp[n]);
> +                strncpy(xattr, tmp[n], len+1);

If you know the length anyway you shiuld probably use memcpy() here.


> +                p = strchr(xattr, '=');
> +                if (!p) {
> +                        log_error("%s: Attribute has incorrect format.", i->path);
> +                        return -EBADMSG;
> +                }
> +                if (p[1] == '\"') {
> +                        while (true) {
> +                                if (!p)
> +                                        p = tmp[n];
> +                                else
> +                                        p += 2;
> +                                p = strchr(p, '\"');
> +                                if (p && xattr[p-xattr-1] != '\\')
> +                                        break;
> +                                p = NULL;
> +                                ++n;
> +                                if (n == strv_len)
> +                                        break;
> +                                len += strlen(tmp[n]) + 1;
> +                                strncat(xattr, " ", 1);
> +                                strncat(xattr, tmp[n], len);
> +                        }
> +                }

strncat() is an awful command. There are always better ways.

In this case FOREACH_WORD_QUOTED sounds like the better option...

> +                strstrip(xattr);
> +                if (strv_extend(&i->xattrs, xattr) < 0)
> +                        return log_oom();

It sounds like a better idea to allocate the new attribute with
strndup() right from the beginning in each loop, and then use
strv_push() to just make it part of the strv...

> +        }
> +
> +        free(i->argument);
> +        i->argument = NULL;

Hmm, did I miss something, why do you explicitly free() that here?
wouldn't that be cleaned up anyway at the end for you?

> +        return 0;
> +}
> +#endif
> +
> +static int item_set_xattrs(Item *i, const char *path) {
> +#ifdef HAVE_XATTR
> +        char **x;
> +        int n;
> +
> +        assert(i);
> +        assert(path);
> +
> +        n = get_xattrs_from_arg(i);
> +        if (n < 0)
> +                return n;

In most cases we call the return code variable "r", and use "n" for
storing the number of items of something. Not that it matters much
though, ...

Otherwise looks OK.

Lennart

-- 
Lennart Poettering, Red Hat