[systemd-devel] [PATCH v2, ping?] tmpfiles, man: Add xattr support to tmpfiles

[Lennart Poettering]

On Mon, 15.07.13 15:22, Maciej Wereski (m.wereski at partner.samsung.com) wrote:

> +                                <varlistentry>
> +                                        <term><varname>t</varname></term>
> +                                        <listitem><para>Set extended
> +                                        attributes on item. It should be
> +                                        used with conjunction with
> other

Well, I'd use the wording "may be used", not "should be used" here...

> +        if (!i->xattrs)
> +                return 0;

We usually use strv_isempty() for that, which checks a bit more, but
this doesn't really matter in this case...

> +        STRV_FOREACH(x, i->xattrs) {
> +                value = *x;
> +                name = strsep(&value, "=");

I'd really prefer if we didn't corrupt the string here. Maybe use
strv_split_quoted() here? That handles all the values for you anyway...

> +                n = strlen(value);
> +                if (i->type == CREATE_SYMLINK) {
> +                        if (lsetxattr(path, name, value, n+1, 0) < 0) {
> +                                log_error("Setting extended attribute %s=%s on symlink %s failed: %m", name, value, path);
> +                                free(value);
> +                                return -errno;
> +                        }
> +                }
> +                else if (setxattr(path, name, value, n+1, 0) < 0) {
> +                        log_error("Setting extended attribute %s=%s on %s failed: %m", name, value, path);
> +                        free(value);
> +                        return -errno;
> +                }
> +                free(value);

Hmm, the two if branches look like something you could combine further:

if (i->type == CREATE_SYMLINK) 
        r = lsetxattr(...);
else
        r = setxattr(...);

And note that log_error() guarantees that errno stays untouched.

> +        if (i->xattrs) {
> +                r = item_set_xattrs(i, i->path);
> +                if (r < 0)
> +                        return r;
> +        }

Checking i->xattrs outside and inside the function is redundand, no?

> +        for (n = 0; n < strv_length(tmp); ++n) {
> +                len = strlen(tmp[n]);
> +                strncpy(xattr, tmp[n], len+1);
> +                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_length(tmp))
> +                                        break;
> +                                len += strlen(tmp[n]) + 1;
> +                                strncat(xattr, " ", 1);
> +                                strncat(xattr, tmp[n], len);
> +                        }
> +                }
> +                strstrip(xattr);
> +                f = i->xattrs;
> +                i->xattrs = strv_append(i->xattrs, xattr);
> +                if (!i->xattrs){
> +                        strv_free(f);
> +                        return log_oom();
> +                }

For this stuf I'd really prefer using one of our already existing
quoting APIs, like strv_spit_quoted() or FOREACH_WORD_QUOTED or so.

>          int r = 0, k;
>          _cleanup_globfree_ glob_t g = {};
> @@ -674,6 +799,12 @@ static int create_item(Item *i) {
>                  if (r < 0)
>                          return r;
>  
> +                if (i->xattrs) {
> +                        r = item_set_xattrs(i, i->path);
> +                        if (r < 0)
> +                                return r;
> +                }
> +

item_set_xattrs already checks i->xattrs for empty anyway, so this could
be shortened quite a bit (as above and a couple of more times)

> +static int copy_item_contents(Item *dest, const Item *src) {

Hmm, not following why you need to copy any items ever. Either you add a
new item, or you patch the existing one, but why ever copy one?

Lennart

-- 
Lennart Poettering - Red Hat, Inc.