Re: [systemd-devel] [PATCH v2, ping?] tmpfiles, man: Add xattr support to tmpfiles
It was <2013-07-15 pon 15:22>, when Maciej Wereski wrote: > diff --git a/man/tmpfiles.d.xml b/man/tmpfiles.d.xml > index 519f9bc..92157b5 100644 > --- a/man/tmpfiles.d.xml > +++ b/man/tmpfiles.d.xml > @@ -229,6 +229,21 @@ L/tmp/foobar ---- > /dev/null > place of normal path > names. > > + > + > +t > +Set extended > +attributes on item. It should be > +used with conjunction with other "in conjunction with" -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v2, ping?] tmpfiles, man: Add xattr support to tmpfiles
On Thu, 18.07.13 17:09, Maciej Wereski (m.were...@partner.samsung.com) wrote:
>
> Hello,
>
> 16.07.2013 at 00:31 Lennart Poettering wrote:
>
> >>+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...
>
> You mean strv_split() (I'm splitting by "=")? This has one issue: it
> splits by all separator occurrences and I need to split after first one.
> If corrupting string is the issue, I can make a copy of it. If you prefer
> strv_split(), then I can just join if strv_length > 2.
Oh, true.
Given that this appears to be recurring theme I have now added a
split_pair() call for you. It will split a string up at a specified
separator into a pair. You could then pass the second argument into
unquote() and that's all you need?
Could you please rework your patch with that?
Lennart
--
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v2, ping?] tmpfiles, man: Add xattr support to tmpfiles
Hello,
16.07.2013 at 00:31 Lennart Poettering wrote:
+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...
You mean strv_split() (I'm splitting by "=")? This has one issue: it
splits by all separator occurrences and I need to split after first one.
If corrupting string is the issue, I can make a copy of it. If you prefer
strv_split(), then I can just join if strv_length > 2.
+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.
Well, I've tried it in the beginning, but in doesn't work properly in this
case. split_quoted() expects quote on the beginning of a string (ignoring
whitespace occurrences). If there's no such case string will be split using
whitespace. Example of extended attribute with quotes:
user.test="This will \" fail"
So how would you like this case to be solved?
regards,
Maciej
--
Maciej Wereski
Samsung R&D Institute Poland
Samsung Electronics
m.were...@partner.samsung.com
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v2, ping?] tmpfiles, man: Add xattr support to tmpfiles
On Mon, 15.07.13 15:22, Maciej Wereski (m.were...@partner.samsung.com) wrote:
> +
> +t
> +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.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
