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
[systemd-devel] [PATCH v2, ping?] tmpfiles, man: Add xattr support to tmpfiles
This patch makes it possible to set extended attributes on files created
by tmpfiles. This can be especially used to set SMACK security labels on
volatile files and directories.
It is done by adding new line of type "t". Such line should contain
attributes in Argument field, using following format:
name=value
All other fields are ignored.
If value contains spaces, then it must be surrounded by quotation marks.
User can also put quotation mark in value by escaping it with backslash.
Example:
D /var/run/cups - - - -
t /var/run/cups - - - - security.SMACK64=printing
---
I've used "t" because IMHO "a" will be better for acl. To sum up: when
"t" is met and it's not in hashmap, then it will be added. Then if other
line for the same file appears, then it replaces SET_XATTR item in
hashmap and has extended attributes added. If item earler existed in
hashmap, then extended attributes are merged to existing entry. This
means that there can be more than one "t" lines for one file. There is
also posibility to have standalone "t" line. I hope that this is desired
behaviour.
regards,
Maciej
---
man/tmpfiles.d.xml | 26 -
src/tmpfiles/tmpfiles.c | 274 ++--
2 files changed, 285 insertions(+), 15 deletions(-)
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
+types (only d, D, f, F, L, p, c, b, z
+makes sense). If used as a standalone
+line, then systemd-tmpfiles
+ will try to set extended
+attributes on specified path.
+This can be especially used to set
+SMACK labels.
+
+
@@ -242,7 +257,7 @@ L/tmp/foobar ----
/dev/null
objects. For z, Z lines if omitted or when set
to - the file access mode will not be
modified. This parameter is ignored for x, r,
-R, L lines.
+R, L, t lines.
@@ -254,7 +269,7 @@ L/tmp/foobar ----
/dev/null
omitted or when set to - the default 0 (root)
is used. For z, Z lines when omitted or when set to -
the file ownership will not be modified.
-These parameters are ignored for x, r, R, L
lines.
+These parameters are ignored for x, r, R, L, t
lines.
@@ -307,8 +322,10 @@ L/tmp/foobar ----
/dev/null
minor formatted as integers, separated by :,
e.g. "1:3". For f, F, w may be used to specify
a short string that is written to the file,
-suffixed by a newline. Ignored for all other
+suffixed by a newline. Fot t determines extended
+attributes to be set. Ignored for all other
lines.
+
@@ -320,7 +337,8 @@ L/tmp/foobar ----
/dev/null
screen needs two directories
created at boot with specific modes and ownership.
d /var/run/screens 1777 root root 10d
-d /var/run/uscreens 0755 root root 10d12h
+d /var/run/uscreens 0755 root root 10d12h
+t /var/run/screen - - - - user.name="John Koval"
security.SMACK64=screen
diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
index 555347a..098413f 100644
--- a/src/tmpfiles/tmpfiles.c
+++ b/src/tmpfiles/tmpfiles.c
@@ -39,6 +39,9 @@
#include
#include
#include
+#ifdef HAVE_XATTR
+#include
+#endif
#include "log.h"
#include "util.h"
@@ -75,7 +78,10 @@ typedef enum ItemType {
REMOVE_PATH = 'r',
RECURSIVE_REMOVE_PATH = 'R',
RELABEL_PATH = 'z',
-RECURSIVE_RELABEL_PATH = 'Z'
+RECURSIVE_RELABEL_PATH = 'Z',
+
+/* These ones are options/additional operations */
+SET_XATTR = 't'
} ItemType;
typedef struct Item {
@@ -83,
