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

[Lennart Poettering]

On Thu, 30.10.14 12:21, Maciej Wereski (m.wereski at partner.samsung.com) wrote:

>  
> +static int get_xattrs_from_arg(Item *i) {
> +        _cleanup_free_ char *xattr = NULL;
> +        const char *p;
> +        int n;
> +
> +        assert(i);
> +        if (i->type != SET_XATTR)
> +                return 0;
> +
> +        if (!i->argument) {
> +                log_error("%s: Argument can't be empty!", i->path);
> +                return -EBADMSG;
> +        }
> +        p = i->argument;
> +
> +        while ((n = unquote_first_word(&p, &xattr)) > 0) {
> +                if (strv_extend(&i->xattrs, xattr) < 0)
> +                        return log_oom();
> +                free(xattr);
> +                xattr = NULL;
> +        }

Please use strv_consume() or strv_push() here, to make the additional
copy unnecessary. Also please, generate a parse failure if
unquote_first_workd() fails due to parse errors.

> +        r = get_xattrs_from_arg(i);
> +        if (r < 0)
> +                return r;
> +
> +        if (strv_isempty(i->xattrs))
> +                return 0;
> +
> +        STRV_FOREACH(x, i->xattrs) {
> +                _cleanup_free_ char *name = NULL, *value = NULL, *tmp = NULL;
> +                n = split_pair(*x, "=", &name, &value);
> +                if (n < 0)
> +                        return n;
> +                tmp = unquote(value, "\"");
> +                if (!tmp)
> +                        return log_oom();
> +                free(value);
> +                value = cunescape(tmp);
> +                if (!value)
> +                        return log_oom();
> +                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);
> +                                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);
> +                        return -errno;
> +                }

The indentation is wrong.

Should the extended attribute value really include the trailing NUL
byte? I doubt it...

Shouldn't we always invoke lsetxattr() instead of setxattr()?

If we really should keep both around, I think a single error handling
for both if blocks would be better.

>  
> @@ -894,6 +983,12 @@ static int create_item(Item *i) {
>                  r = glob_item(i, item_set_perms);
>                  if (r < 0)
>                          return r;
> +
> +                if (i->xattrs) {
> +                        r = glob_item(i, item_set_xattrs);
> +                        if (r < 0)
> +                                return r;
> +                }
>                  break;

Hmm, ths would mean we resolve the globbing twice. Once for
item_set_perms() and once for item_set_xattr(). I think it would be
better to do this in one call.

>  
>          case RECURSIVE_RELABEL_PATH:
> @@ -901,8 +996,12 @@ static int create_item(Item *i) {
>                  r = glob_item(i, item_set_perms_recursive);
>                  if (r < 0)
>                          return r;
> -
>                  break;
> +
> +        case SET_XATTR:
> +                r = item_set_xattrs(i, i->path);
> +                if (r < 0)
> +                        return r;
>          }


Misses the closing "break;"?

Looks OK otherwise.

Lennart

-- 
Lennart Poettering, Red Hat