Re: [systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-04-13 Thread Lennart Poettering 
On Sun, 12.04.15 22:19, Goffredo Baroncelli (kreij...@libero.it) wrote:

 However the original code catch also the case where the file is a soft-link.
 The same check is performed also by chattr(1); I suggest to leave the original
 behavior, changing
 
 fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
 in
   fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_NOFOLLOW);
 
 and checking if the errno is ELOOP. In this case a further check is performed 
 to 
 verify if the file is a link or the error is due to a too many symbolic link.
 Then an appropriate message error is printed.
 
 What do you think ?

We should probably either follow symlinks for all of tmpfiles'
operations or for none. 

While I generally believe that we probably shouldn't follow symlinks,
it's really difficult to implement given that fchmodat() currenlty
doesn't work with AT_SYMLINK_FOLLOW (according to the man page at
least), and acl_set_file doesn't allow not following symlinks either... :-(

Hmm, I can't say I like this I must say.

ideas?

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-04-13 Thread Lennart Poettering 
On Mon, 13.04.15 11:31, Lennart Poettering (lenn...@poettering.net) wrote:

 On Sun, 12.04.15 22:19, Goffredo Baroncelli (kreij...@libero.it) wrote:
 
  However the original code catch also the case where the file is a soft-link.
  The same check is performed also by chattr(1); I suggest to leave the 
  original
  behavior, changing
  
  fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
  in
  fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_NOFOLLOW);
  
  and checking if the errno is ELOOP. In this case a further check is 
  performed to 
  verify if the file is a link or the error is due to a too many symbolic 
  link.
  Then an appropriate message error is printed.
  
  What do you think ?
 
 We should probably either follow symlinks for all of tmpfiles'
 operations or for none. 
 
 While I generally believe that we probably shouldn't follow symlinks,
 it's really difficult to implement given that fchmodat() currenlty
 doesn't work with AT_SYMLINK_FOLLOW (according to the man page at
 least), and acl_set_file doesn't allow not following symlinks either... :-(
 
 Hmm, I can't say I like this I must say.
 
 ideas?

I now fixed much of the code to not follow symlinks anymore. With a
combination of O_PATH and using /proc/self/fd/%i for acl_set_file() I
managed to get all of the xattr, acl, file attribute, chmod, chown
code to not follow symlinks anymore.

I also documented this behaviour in the man page for the lines where
this applies.

I am pretty sure we should not follow symlinks when creating new file
objects or adjusting existing ones (with the notable exception of w
lines). Right now we still follow symlinks when creating dirs, fifos,
device nodes. We should fix that too.

Anyway, Goffredo, the file attribute code will not follow symlinks
anymore, hence this should settle this issue you raised.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-04-12 Thread Goffredo Baroncelli 
Hi Lennart,

On 2015-04-08 20:36, Lennart Poettering wrote:
 +
  +static int path_set_attrib(Item *item, const char *path) {
  +_cleanup_close_ int fd = -1;
  +int r;
  +unsigned f;
  +struct stat st;
  +
  +/* do nothing */
  +if (item-attrib_mask == 0 || !item-attrib_set)
  +return 0;
  +/*
  + * It is OK to ignore an lstat() error, because the error
  + * will be catch by the open() below anyway
  + */
  +if (lstat(path, st) == 0 
  +!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
  +return 0;
  +}
 Why do we need this check? What happens if we apply this onto
 a device node, or socket, or whatever? 
 
 I really don#t like this seperate lstat() + open(). If it all it
 should be open() + fstat(), to avoid TTOCTOU issues...

I am checking your changes; you modified the code above in :

fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
if (fd  0)
return log_error_errno(errno, Cannot open '%s': %m, path);

if (fstat(fd, st)  0)
return log_error_errno(errno, Cannot stat '%s': %m, path);

/* Issuing the file attribute ioctls on device nodes is not
 * safe, as that will be delivered to the drivers, not the
 * file system containing the device node. */
if (!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
log_error(Setting file flags is only supported on regular 
files and directories, cannot set on '%s'., path);
return -EINVAL;
}


However the original code catch also the case where the file is a soft-link.
The same check is performed also by chattr(1); I suggest to leave the original
behavior, changing

fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
in
fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_NOFOLLOW);

and checking if the errno is ELOOP. In this case a further check is performed 
to 
verify if the file is a link or the error is due to a too many symbolic link.
Then an appropriate message error is printed.

What do you think ?

Goffredo

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli kreijackATinwind.it
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-04-08 Thread Lennart Poettering 
On Mon, 16.03.15 20:33, Goffredo Baroncelli (kreij...@libero.it) wrote:

Sorry for warming up this old thread, but a few issues I see with this?

  RECURSIVE_RELABEL_PATH = 'Z',
 +SET_ATTRIB = 'h',
 +RECURSIVE_SET_ATTRIB = 'H',
  } ItemType;
  
  typedef struct Item {
 @@ -108,12 +111,15 @@ typedef struct Item {
  usec_t age;
  
  dev_t major_minor;
 +int attrib_value;
 +int attrib_mask;

I thought this was unsigned now?

  
 +static int get_attrib_from_arg(Item *item) {
 +static const unsigned attributes[] = {
 +[(uint8_t)'A'] = FS_NOATIME_FL,/* do not update atime */
 +[(uint8_t)'S'] = FS_SYNC_FL,  /* Synchronous updates */
 +[(uint8_t)'D'] = FS_DIRSYNC_FL,   /* dirsync behaviour 
 (directories only) */
 +[(uint8_t)'a'] = FS_APPEND_FL,/* writes to file may only 
 append */
 +[(uint8_t)'c'] = FS_COMPR_FL, /* Compress file */
 +[(uint8_t)'d'] = FS_NODUMP_FL,/* do not dump file */
 +[(uint8_t)'e'] = FS_EXTENT_FL,/* Top of directory 
 hierarchies*/
 +[(uint8_t)'i'] = FS_IMMUTABLE_FL, /* Immutable file */
 +[(uint8_t)'j'] = FS_JOURNAL_DATA_FL, /* Reserved for ext3 */
 +[(uint8_t)'s'] = FS_SECRM_FL, /* Secure deletion */
 +[(uint8_t)'u'] = FS_UNRM_FL,  /* Undelete */
 +[(uint8_t)'t'] = FS_NOTAIL_FL,/* file tail should not be 
 merged */
 +[(uint8_t)'T'] = FS_TOPDIR_FL,/* Top of directory 
 hierarchies*/
 +[(uint8_t)'C'] = FS_NOCOW_FL, /* Do not cow file */
 +};

Hmm, that's quite a sparse array! It wastes 116 entries, just to
store 13 entries... I think this should really be an array fo structs
with the chars and their flag, which we iterate through... 

 +const unsigned all_flags = FS_NOATIME_FL|FS_SYNC_FL|FS_DIRSYNC_FL|
 +FS_APPEND_FL|FS_COMPR_FL|FS_NODUMP_FL|FS_EXTENT_FL|
 +FS_IMMUTABLE_FL|FS_JOURNAL_DATA_FL|FS_SECRM_FL|FS_UNRM_FL|
 +FS_NOTAIL_FL|FS_TOPDIR_FL|FS_NOCOW_FL;
 +char *p = item-argument;
 +enum {
 +MODE_ADD,
 +MODE_DEL,
 +MODE_SET
 +} mode = MODE_ADD;
 +unsigned long value = 0, mask = 0;

And now it is unsigned long, not just unsigned anymore?

 +
 +static int path_set_attrib(Item *item, const char *path) {
 +_cleanup_close_ int fd = -1;
 +int r;
 +unsigned f;
 +struct stat st;
 +
 +/* do nothing */
 +if (item-attrib_mask == 0 || !item-attrib_set)
 +return 0;
 +/*
 + * It is OK to ignore an lstat() error, because the error
 + * will be catch by the open() below anyway
 + */
 +if (lstat(path, st) == 0 
 +!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
 +return 0;
 +}

Why do we need this check? What happens if we apply this onto
a device node, or socket, or whatever? 

I really don#t like this seperate lstat() + open(). If it all it
should be open() + fstat(), to avoid TTOCTOU issues...

 +fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
 +
 +if (fd  0)
 +return log_error_errno(errno, Cannot open \%s\: %m, 
 path);
  
 +case SET_ATTRIB:
 +case RECURSIVE_SET_ATTRIB:
 +if (!i.argument) {
 +log_error([%s:%u] Set attrib requires
 argument., fname, line);

Please don't abbreviate words like attrib in human language...

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-04-08 Thread Goffredo Baroncelli 
On 2015-04-08 20:36, Lennart Poettering wrote:
 On Mon, 16.03.15 20:33, Goffredo Baroncelli (kreij...@libero.it) wrote:
 
 Sorry for warming up this old thread, but a few issues I see with this?
 
  RECURSIVE_RELABEL_PATH = 'Z',
 +SET_ATTRIB = 'h',
 +RECURSIVE_SET_ATTRIB = 'H',
  } ItemType;
  
  typedef struct Item {
 @@ -108,12 +111,15 @@ typedef struct Item {
  usec_t age;
  
  dev_t major_minor;
 +int attrib_value;
 +int attrib_mask;
 
 I thought this was unsigned now?

Sew below



 
  
 +static int get_attrib_from_arg(Item *item) {
 +static const unsigned attributes[] = {
 +[(uint8_t)'A'] = FS_NOATIME_FL,/* do not update atime */
 +[(uint8_t)'S'] = FS_SYNC_FL,  /* Synchronous updates */
 +[(uint8_t)'D'] = FS_DIRSYNC_FL,   /* dirsync behaviour 
 (directories only) */
 +[(uint8_t)'a'] = FS_APPEND_FL,/* writes to file may 
 only append */
 +[(uint8_t)'c'] = FS_COMPR_FL, /* Compress file */
 +[(uint8_t)'d'] = FS_NODUMP_FL,/* do not dump file */
 +[(uint8_t)'e'] = FS_EXTENT_FL,/* Top of directory 
 hierarchies*/
 +[(uint8_t)'i'] = FS_IMMUTABLE_FL, /* Immutable file */
 +[(uint8_t)'j'] = FS_JOURNAL_DATA_FL, /* Reserved for ext3 */
 +[(uint8_t)'s'] = FS_SECRM_FL, /* Secure deletion */
 +[(uint8_t)'u'] = FS_UNRM_FL,  /* Undelete */
 +[(uint8_t)'t'] = FS_NOTAIL_FL,/* file tail should not 
 be merged */
 +[(uint8_t)'T'] = FS_TOPDIR_FL,/* Top of directory 
 hierarchies*/
 +[(uint8_t)'C'] = FS_NOCOW_FL, /* Do not cow file */
 +};
 
 Hmm, that's quite a sparse array! It wastes 116 entries, just to
 store 13 entries... I think this should really be an array fo structs
 with the chars and their flag, which we iterate through... 

I accepted a suggestion of Zbyszek; this solution avoids to loop over the 
list... 
 
 +const unsigned all_flags = FS_NOATIME_FL|FS_SYNC_FL|FS_DIRSYNC_FL|
 +FS_APPEND_FL|FS_COMPR_FL|FS_NODUMP_FL|FS_EXTENT_FL|
 +FS_IMMUTABLE_FL|FS_JOURNAL_DATA_FL|FS_SECRM_FL|FS_UNRM_FL|
 +FS_NOTAIL_FL|FS_TOPDIR_FL|FS_NOCOW_FL;
 +char *p = item-argument;
 +enum {
 +MODE_ADD,
 +MODE_DEL,
 +MODE_SET
 +} mode = MODE_ADD;
 +unsigned long value = 0, mask = 0;
 
 And now it is unsigned long, not just unsigned anymore?

For 32 bit, the ioctl FS_IOC_GETFLAGS seems to be defined with an int as 
argument; for 64bit, the argument is a long; however chattr_fd() uses 
unsigned... Yes this is a mess; probably int is a good answer

 
 +
 +static int path_set_attrib(Item *item, const char *path) {
 +_cleanup_close_ int fd = -1;
 +int r;
 +unsigned f;
 +struct stat st;
 +
 +/* do nothing */
 +if (item-attrib_mask == 0 || !item-attrib_set)
 +return 0;
 +/*
 + * It is OK to ignore an lstat() error, because the error
 + * will be catch by the open() below anyway
 + */
 +if (lstat(path, st) == 0 
 +!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
 +return 0;
 +}
 
 Why do we need this check? What happens if we apply this onto
 a device node, or socket, or whatever? 

I copied this check from the source of chattr; the reason is:

(from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=152029#10)
[...]
$ lsattr /dev/dri/card0
s-S--- /dev/dri/card0
Segmentation fault (core dumped)

$ ls -al /dev/dri/card0
crw-rw-rw-1 root root 226,   0 jun 30 11:12 /dev/dri/card0

lsattr and chattr can not work against device files, since they use
ioctl's which are supported by the ext2 filesystem, but which aren't
supported by normal devices (and unfortunately normal devices don't
forwawrd the ioctl on to the ext2 filesystem).  In this case, the ioctl
used by lsattr is apparently also implemented by the DRI device driver,
but probably does something completely different.  So the returned
device structure contains garbage, which causes lsattr to core dump
[...]


 
 I really don#t like this seperate lstat() + open(). If it all it
 should be open() + fstat(), to avoid TTOCTOU issues...
 
 +fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
 +
 +if (fd  0)
 +return log_error_errno(errno, Cannot open \%s\: %m, 
 path);
  
 +case SET_ATTRIB:
 +case RECURSIVE_SET_ATTRIB:
 +if (!i.argument) {
 +log_error([%s:%u] Set attrib requires
 argument., fname, line);
 
 Please don't abbreviate words like attrib in human language...
 
 Lennart
 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli kreijackATinwind.it
Key fingerprint BBF5 1610 0B64 DAC6 5F7D

Re: [systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-04-08 Thread Lennart Poettering 
On Wed, 08.04.15 21:21, Goffredo Baroncelli (kreij...@libero.it) wrote:

  +const unsigned all_flags = FS_NOATIME_FL|FS_SYNC_FL|FS_DIRSYNC_FL|
  +FS_APPEND_FL|FS_COMPR_FL|FS_NODUMP_FL|FS_EXTENT_FL|
  +FS_IMMUTABLE_FL|FS_JOURNAL_DATA_FL|FS_SECRM_FL|FS_UNRM_FL|
  +FS_NOTAIL_FL|FS_TOPDIR_FL|FS_NOCOW_FL;
  +char *p = item-argument;
  +enum {
  +MODE_ADD,
  +MODE_DEL,
  +MODE_SET
  +} mode = MODE_ADD;
  +unsigned long value = 0, mask = 0;
  
  And now it is unsigned long, not just unsigned anymore?
 
 For 32 bit, the ioctl FS_IOC_GETFLAGS seems to be defined with an
 int as argument; for 64bit, the argument is a long; however
 chattr_fd() uses unsigned... Yes this is a mess; probably int is a
 good answer

Well, the kernel is a bit confused, but as far as I can see it
generally treats it as unsigned. Only in the ext234 code there's
this gem that it calls get_user() and pretends it was an int, while
reading it into an unsigned.  btrfs never falls for that, and neither
do most other file systems afaics.

e2fsprogs is also confused. It treats it mostly as unsigned long,
but then converts it to int right before issueing the ioctl. 

I guess the kernel devs haven't understood the concept of type
safety yet, or even worse, of types ;-)

But anway, I'd stay close to the kernel, so let's make this an
unsigned.

Flag fields that are signed would be weird anyway...

  +if (lstat(path, st) == 0 
  +!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
  +return 0;
  +}
  
  Why do we need this check? What happens if we apply this onto
  a device node, or socket, or whatever? 
 
 I copied this check from the source of chattr; the reason is:
 
 (from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=152029#10)
 [...]
 $ lsattr /dev/dri/card0
 s-S--- /dev/dri/card0
 Segmentation fault (core dumped)
 
 $ ls -al /dev/dri/card0
 crw-rw-rw-1 root root 226,   0 jun 30 11:12 /dev/dri/card0
 
 lsattr and chattr can not work against device files, since they use
 ioctl's which are supported by the ext2 filesystem, but which aren't
 supported by normal devices (and unfortunately normal devices don't
 forwawrd the ioctl on to the ext2 filesystem).  In this case, the ioctl
 used by lsattr is apparently also implemented by the DRI device driver,
 but probably does something completely different.  So the returned
 device structure contains garbage, which causes lsattr to core dump
 [...]

I see, this is indeed a real problem (and in fact we might need to fix
this for a couple of other fs related ioctls, too).

Either way, this really should be an fstat() on the opened file and
not a stat() before, due to TOCTTOU.

Anyway, I now made the necessary changes, and changed a few other
things too, and gave it some testing. Please have a look if it still
looks good to you!

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-03-16 Thread Goffredo Baroncelli 
On 2015-03-16 04:12, Zbigniew Jędrzejewski-Szmek wrote:
 On Tue, Mar 10, 2015 at 09:07:41PM +0100, Goffredo Baroncelli wrote:
 Allow systemd-tmpfiles to set the file/directory attributes, like chattr(1)
 does. Two more commands are added: 'H' and 'h' to set the attributes,
 recursively and not.
 ---
  src/tmpfiles/tmpfiles.c | 140 
 
  1 file changed, 140 insertions(+)

 diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
 index c948d4d..165605f 100644
 --- a/src/tmpfiles/tmpfiles.c
 +++ b/src/tmpfiles/tmpfiles.c
 @@ -40,6 +40,7 @@
  #include sys/types.h
  #include sys/param.h
  #include sys/xattr.h
 +#include linux/fs.h
  
  #include log.h
  #include util.h
 @@ -90,6 +91,8 @@ typedef enum ItemType {
  ADJUST_MODE = 'm', /* legacy, 'z' is identical to this */
  RELABEL_PATH = 'z',
  RECURSIVE_RELABEL_PATH = 'Z',
 +SET_ATTRIB = 'h',
 +RECURSIVE_SET_ATTRIB = 'H',
  } ItemType;
  
  typedef struct Item {
 @@ -108,12 +111,15 @@ typedef struct Item {
  usec_t age;
  
  dev_t major_minor;
 +int attrib_value;
 +int attrib_mask;
  
  bool uid_set:1;
  bool gid_set:1;
  bool mode_set:1;
  bool age_set:1;
  bool mask_perms:1;
 +bool attrib_set:1;
  
  bool keep_first_level:1;
  
 @@ -762,6 +768,115 @@ static int path_set_acls(Item *item, const char *path) 
 {
  return 0;
  }
  
 +static int get_attrib_from_arg(Item *item) {
 +static const struct { int value; char ch; } attributes[] = {
 +{ FS_NOATIME_FL, 'A' },   /* do not update atime */
 +{ FS_SYNC_FL, 'S' },  /* Synchronous updates */
 +{ FS_DIRSYNC_FL, 'D' },   /* dirsync behaviour (directories 
 only) */
 +{ FS_APPEND_FL, 'a' },/* writes to file may only append 
 */
 +{ FS_COMPR_FL, 'c' }, /* Compress file */
 +{ FS_NODUMP_FL, 'd' },/* do not dump file */
 +{ FS_EXTENT_FL, 'e'}, /* Top of directory hierarchies*/
 +{ FS_IMMUTABLE_FL, 'i' }, /* Immutable file */
 +{ FS_JOURNAL_DATA_FL, 'j' }, /* Reserved for ext3 */
 +{ FS_SECRM_FL, 's' }, /* Secure deletion */
 +{ FS_UNRM_FL, 'u' },  /* Undelete */
 +{ FS_NOTAIL_FL, 't' },/* file tail should not be merged 
 */
 +{ FS_TOPDIR_FL, 'T' },/* Top of directory hierarchies*/
 +{ FS_NOCOW_FL, 'C' }, /* Do not cow file */
 +{ 0, 0 }
 +};
 +char *p = item-argument;
 +enum {
 +MODE_ADD,
 +MODE_DEL,
 +MODE_SET
 +} mode = MODE_ADD;
 +unsigned long value = 0, mask = 0;
 +
 +if (!p) {
 +log_error(\%s\: setting ATTR need an argument, 
 item-path);
 +return -EINVAL;
 +}
 +
 +if (*p == '+') {
 +mode = MODE_ADD;
 +p++;
 +} else if (*p == '-') {
 +mode = MODE_DEL;
 +p++;
 +} else  if (*p == '=') {
 +mode = MODE_SET;
 +p++;
 +}
 +
 +if (!*p  mode != MODE_SET) {
 +log_error(\%s\: setting ATTR: argument is empty, 
 item-path);
 +return -EINVAL;
 +}
 +for (; *p ; p++) {
 +int i;
 +for (i = 0; attributes[i].ch  attributes[i].ch != *p; i++)
 +;
 Why not order the table the other way:
 
 static const int attributes[] = {
[(uint8_t)'A'] = FS_NOATIME_FL,  /* do not update atime */
...
 };
 
 if ((uint8_t)*p  ELEMENTSOF(attributes) || attributes[(uint8_t)*p] 
 == 0)
 log_error(\%s\: setting ATTR: unknown attr '%c', 
 item-path, *p);
 return -EINVAL;
 }
 
 Not looping is always nicer.

I find this idea very elegant, my only concern was the memory occupation: the 
ascii code
for the 't' letter is about 120, this means that the array will have a size of 
~240 bytes...
Ok that the PC have several GBs


 
 +if (!attributes[i].ch) {
 +log_error(\%s\: setting ATTR: unknown attr 
 '%c', item-path, *p);
 +return -EINVAL;
 +}
 +if (mode == MODE_ADD || mode == MODE_SET)
 +value |= attributes[i].value;
 +else
 +value = ~attributes[i].value;
 +mask |= attributes[i].value;
 +}
 +
 +if (mode == MODE_SET) {
 +int i;
 +for (i = 0; attributes[i].ch; i++)
 +mask |= attributes[i].value;
 This simply FS_NOATIME_FL|FS_SYNC_FL|... Even if the compiler can optimize

[systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-03-16 Thread Goffredo Baroncelli 
From: Goffredo Baroncelli kreij...@inwind.it

Allow systemd-tmpfiles to set the file/directory attributes, like chattr(1)
does. Two more commands are added: 'H' and 'h' to set the attributes,
recursively and not.
---
 src/tmpfiles/tmpfiles.c | 140 
 1 file changed, 140 insertions(+)

diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
index c948d4d..e1a22f5 100644
--- a/src/tmpfiles/tmpfiles.c
+++ b/src/tmpfiles/tmpfiles.c
@@ -40,6 +40,7 @@
 #include sys/types.h
 #include sys/param.h
 #include sys/xattr.h
+#include linux/fs.h
 
 #include log.h
 #include util.h
@@ -90,6 +91,8 @@ typedef enum ItemType {
 ADJUST_MODE = 'm', /* legacy, 'z' is identical to this */
 RELABEL_PATH = 'z',
 RECURSIVE_RELABEL_PATH = 'Z',
+SET_ATTRIB = 'h',
+RECURSIVE_SET_ATTRIB = 'H',
 } ItemType;
 
 typedef struct Item {
@@ -108,12 +111,15 @@ typedef struct Item {
 usec_t age;
 
 dev_t major_minor;
+int attrib_value;
+int attrib_mask;
 
 bool uid_set:1;
 bool gid_set:1;
 bool mode_set:1;
 bool age_set:1;
 bool mask_perms:1;
+bool attrib_set:1;
 
 bool keep_first_level:1;
 
@@ -762,6 +768,115 @@ static int path_set_acls(Item *item, const char *path) {
 return 0;
 }
 
+static int get_attrib_from_arg(Item *item) {
+static const unsigned attributes[] = {
+[(uint8_t)'A'] = FS_NOATIME_FL,/* do not update atime */
+[(uint8_t)'S'] = FS_SYNC_FL,  /* Synchronous updates */
+[(uint8_t)'D'] = FS_DIRSYNC_FL,   /* dirsync behaviour 
(directories only) */
+[(uint8_t)'a'] = FS_APPEND_FL,/* writes to file may only 
append */
+[(uint8_t)'c'] = FS_COMPR_FL, /* Compress file */
+[(uint8_t)'d'] = FS_NODUMP_FL,/* do not dump file */
+[(uint8_t)'e'] = FS_EXTENT_FL,/* Top of directory 
hierarchies*/
+[(uint8_t)'i'] = FS_IMMUTABLE_FL, /* Immutable file */
+[(uint8_t)'j'] = FS_JOURNAL_DATA_FL, /* Reserved for ext3 */
+[(uint8_t)'s'] = FS_SECRM_FL, /* Secure deletion */
+[(uint8_t)'u'] = FS_UNRM_FL,  /* Undelete */
+[(uint8_t)'t'] = FS_NOTAIL_FL,/* file tail should not be 
merged */
+[(uint8_t)'T'] = FS_TOPDIR_FL,/* Top of directory 
hierarchies*/
+[(uint8_t)'C'] = FS_NOCOW_FL, /* Do not cow file */
+};
+const unsigned all_flags = FS_NOATIME_FL|FS_SYNC_FL|FS_DIRSYNC_FL|
+FS_APPEND_FL|FS_COMPR_FL|FS_NODUMP_FL|FS_EXTENT_FL|
+FS_IMMUTABLE_FL|FS_JOURNAL_DATA_FL|FS_SECRM_FL|FS_UNRM_FL|
+FS_NOTAIL_FL|FS_TOPDIR_FL|FS_NOCOW_FL;
+char *p = item-argument;
+enum {
+MODE_ADD,
+MODE_DEL,
+MODE_SET
+} mode = MODE_ADD;
+unsigned long value = 0, mask = 0;
+
+if (!p) {
+log_error(\%s\: setting ATTR need an argument, item-path);
+return -EINVAL;
+}
+
+if (*p == '+') {
+mode = MODE_ADD;
+p++;
+} else if (*p == '-') {
+mode = MODE_DEL;
+p++;
+} else  if (*p == '=') {
+mode = MODE_SET;
+p++;
+}
+
+if (!*p  mode != MODE_SET) {
+log_error(\%s\: setting ATTR: argument is empty, 
item-path);
+return -EINVAL;
+}
+for (; *p ; p++) {
+if ((uint8_t)*p  ELEMENTSOF(attributes) || 
attributes[(uint8_t)*p] == 0) {
+log_error(\%s\: setting ATTR: unknown attr '%c', 
item-path, *p);
+return -EINVAL;
+}
+if (mode == MODE_ADD || mode == MODE_SET)
+value |= attributes[(uint8_t)*p];
+else
+value = ~attributes[(uint8_t)*p];
+mask |= attributes[(uint8_t)*p];
+}
+
+if (mode == MODE_SET)
+mask |= all_flags;
+
+assert(mask);
+
+item-attrib_mask = mask;
+item-attrib_value = value;
+item-attrib_set = true;
+
+return 0;
+
+}
+
+static int path_set_attrib(Item *item, const char *path) {
+_cleanup_close_ int fd = -1;
+int r;
+unsigned f;
+struct stat st;
+
+/* do nothing */
+if (item-attrib_mask == 0 || !item-attrib_set)
+return 0;
+/*
+ * It is OK to ignore an lstat() error, because the error
+ * will be catch by the open() below anyway
+ */
+if (lstat(path, st) == 0 
+!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
+return 0;
+}
+
+fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
+
+

Re: [systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-03-15 Thread Zbigniew Jędrzejewski-Szmek 
On Tue, Mar 10, 2015 at 09:07:41PM +0100, Goffredo Baroncelli wrote:
 Allow systemd-tmpfiles to set the file/directory attributes, like chattr(1)
 does. Two more commands are added: 'H' and 'h' to set the attributes,
 recursively and not.
 ---
  src/tmpfiles/tmpfiles.c | 140 
 
  1 file changed, 140 insertions(+)
 
 diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
 index c948d4d..165605f 100644
 --- a/src/tmpfiles/tmpfiles.c
 +++ b/src/tmpfiles/tmpfiles.c
 @@ -40,6 +40,7 @@
  #include sys/types.h
  #include sys/param.h
  #include sys/xattr.h
 +#include linux/fs.h
  
  #include log.h
  #include util.h
 @@ -90,6 +91,8 @@ typedef enum ItemType {
  ADJUST_MODE = 'm', /* legacy, 'z' is identical to this */
  RELABEL_PATH = 'z',
  RECURSIVE_RELABEL_PATH = 'Z',
 +SET_ATTRIB = 'h',
 +RECURSIVE_SET_ATTRIB = 'H',
  } ItemType;
  
  typedef struct Item {
 @@ -108,12 +111,15 @@ typedef struct Item {
  usec_t age;
  
  dev_t major_minor;
 +int attrib_value;
 +int attrib_mask;
  
  bool uid_set:1;
  bool gid_set:1;
  bool mode_set:1;
  bool age_set:1;
  bool mask_perms:1;
 +bool attrib_set:1;
  
  bool keep_first_level:1;
  
 @@ -762,6 +768,115 @@ static int path_set_acls(Item *item, const char *path) {
  return 0;
  }
  
 +static int get_attrib_from_arg(Item *item) {
 +static const struct { int value; char ch; } attributes[] = {
 +{ FS_NOATIME_FL, 'A' },   /* do not update atime */
 +{ FS_SYNC_FL, 'S' },  /* Synchronous updates */
 +{ FS_DIRSYNC_FL, 'D' },   /* dirsync behaviour (directories 
 only) */
 +{ FS_APPEND_FL, 'a' },/* writes to file may only append 
 */
 +{ FS_COMPR_FL, 'c' }, /* Compress file */
 +{ FS_NODUMP_FL, 'd' },/* do not dump file */
 +{ FS_EXTENT_FL, 'e'}, /* Top of directory hierarchies*/
 +{ FS_IMMUTABLE_FL, 'i' }, /* Immutable file */
 +{ FS_JOURNAL_DATA_FL, 'j' }, /* Reserved for ext3 */
 +{ FS_SECRM_FL, 's' }, /* Secure deletion */
 +{ FS_UNRM_FL, 'u' },  /* Undelete */
 +{ FS_NOTAIL_FL, 't' },/* file tail should not be merged 
 */
 +{ FS_TOPDIR_FL, 'T' },/* Top of directory hierarchies*/
 +{ FS_NOCOW_FL, 'C' }, /* Do not cow file */
 +{ 0, 0 }
 +};
 +char *p = item-argument;
 +enum {
 +MODE_ADD,
 +MODE_DEL,
 +MODE_SET
 +} mode = MODE_ADD;
 +unsigned long value = 0, mask = 0;
 +
 +if (!p) {
 +log_error(\%s\: setting ATTR need an argument, 
 item-path);
 +return -EINVAL;
 +}
 +
 +if (*p == '+') {
 +mode = MODE_ADD;
 +p++;
 +} else if (*p == '-') {
 +mode = MODE_DEL;
 +p++;
 +} else  if (*p == '=') {
 +mode = MODE_SET;
 +p++;
 +}
 +
 +if (!*p  mode != MODE_SET) {
 +log_error(\%s\: setting ATTR: argument is empty, 
 item-path);
 +return -EINVAL;
 +}
 +for (; *p ; p++) {
 +int i;
 +for (i = 0; attributes[i].ch  attributes[i].ch != *p; i++)
 +;
Why not order the table the other way:

static const int attributes[] = {
   [(uint8_t)'A'] = FS_NOATIME_FL,  /* do not update atime */
   ...
};

if ((uint8_t)*p  ELEMENTSOF(attributes) || attributes[(uint8_t)*p] == 
0)
log_error(\%s\: setting ATTR: unknown attr '%c', 
item-path, *p);
return -EINVAL;
}

Not looping is always nicer.

 +if (!attributes[i].ch) {
 +log_error(\%s\: setting ATTR: unknown attr '%c', 
 item-path, *p);
 +return -EINVAL;
 +}
 +if (mode == MODE_ADD || mode == MODE_SET)
 +value |= attributes[i].value;
 +else
 +value = ~attributes[i].value;
 +mask |= attributes[i].value;
 +}
 +
 +if (mode == MODE_SET) {
 +int i;
 +for (i = 0; attributes[i].ch; i++)
 +mask |= attributes[i].value;
This simply FS_NOATIME_FL|FS_SYNC_FL|... Even if the compiler can optimize
that away, it seems better to just write the OR expression in full rather
than loop.

 +}
 +
 +assert(mask);
 +
 +item-attrib_mask = mask;
 +item-attrib_value = value;
 +item-attrib_set = true;
 +
 +return 0;
 +
 +}
 +
 +static int

[systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-03-12 Thread Goffredo Baroncelli 
Allow systemd-tmpfiles to set the file/directory attributes, like chattr(1)
does. Two more commands are added: 'H' and 'h' to set the attributes,
recursively and not.
---
 src/tmpfiles/tmpfiles.c | 140 
 1 file changed, 140 insertions(+)

diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
index c948d4d..31faeb6 100644
--- a/src/tmpfiles/tmpfiles.c
+++ b/src/tmpfiles/tmpfiles.c
@@ -40,6 +40,7 @@
 #include sys/types.h
 #include sys/param.h
 #include sys/xattr.h
+#include linux/fs.h
 
 #include log.h
 #include util.h
@@ -90,6 +91,8 @@ typedef enum ItemType {
 ADJUST_MODE = 'm', /* legacy, 'z' is identical to this */
 RELABEL_PATH = 'z',
 RECURSIVE_RELABEL_PATH = 'Z',
+SET_ATTRIB = 'h',
+RECURSIVE_SET_ATTRIB = 'H',
 } ItemType;
 
 typedef struct Item {
@@ -108,12 +111,15 @@ typedef struct Item {
 usec_t age;
 
 dev_t major_minor;
+int attrib_value;
+int attrib_mask;
 
 bool uid_set:1;
 bool gid_set:1;
 bool mode_set:1;
 bool age_set:1;
 bool mask_perms:1;
+bool attrib_set:1;
 
 bool keep_first_level:1;
 
@@ -762,6 +768,115 @@ static int path_set_acls(Item *item, const char *path) {
 return 0;
 }
 
+static int get_attrib_from_arg(Item *item) {
+static const struct { int value; char ch; } attributes[] = {
+{ FS_NOATIME_FL, 'A' },   /* do not update atime */
+{ FS_SYNC_FL, 'S' },  /* Synchronous updates */
+{ FS_DIRSYNC_FL, 'D' },   /* dirsync behaviour (directories 
only) */
+{ FS_APPEND_FL, 'a' },/* writes to file may only append */
+{ FS_COMPR_FL, 'c' }, /* Compress file */
+{ FS_NODUMP_FL, 'd' },/* do not dump file */
+{ FS_EXTENT_FL, 'e'}, /* Top of directory hierarchies*/
+{ FS_IMMUTABLE_FL, 'i' }, /* Immutable file */
+{ FS_JOURNAL_DATA_FL, 'j' }, /* Reserved for ext3 */
+{ FS_SECRM_FL, 's' }, /* Secure deletion */
+{ FS_UNRM_FL, 'u' },  /* Undelete */
+{ FS_NOTAIL_FL, 't' },/* file tail should not be merged */
+{ FS_TOPDIR_FL, 'T' },/* Top of directory hierarchies*/
+{ FS_NOCOW_FL, 'C' }, /* Do not cow file */
+{ 0, 0 }
+};
+char *p = item-argument;
+enum {
+MODE_ADD,
+MODE_DEL,
+MODE_SET
+} mode = MODE_ADD;
+unsigned long value = 0, mask = 0;
+
+if (!p) {
+log_error(\%s\: setting ATTR need an argument, item-path);
+return -EINVAL;
+}
+
+if (*p == '+') {
+mode = MODE_ADD;
+p++;
+} else if (*p == '-') {
+mode = MODE_DEL;
+p++;
+} else  if (*p == '=') {
+mode = MODE_SET;
+p++;
+}
+
+if (!*p  mode != MODE_SET) {
+log_error(\%s\: setting ATTR: argument is empty, 
item-path);
+return -EINVAL;
+}
+for (; *p ; p++) {
+int i;
+for (i = 0; attributes[i].ch  attributes[i].ch != *p; i++)
+;
+if (!attributes[i].ch) {
+log_error(\%s\: setting ATTR: unknown attr '%c', 
item-path, *p);
+return -EINVAL;
+}
+if (mode == MODE_ADD || mode == MODE_SET)
+value |= attributes[i].value;
+else
+value = ~attributes[i].value;
+mask |= attributes[i].value;
+}
+
+if (mode == MODE_SET) {
+int i;
+for (i = 0; attributes[i].ch; i++)
+mask |= attributes[i].value;
+}
+
+assert(mask);
+
+item-attrib_mask = mask;
+item-attrib_value = value;
+item-attrib_set = true;
+
+return 0;
+
+}
+
+static int path_set_attrib(Item *item, const char *path) {
+_cleanup_close_ int fd = -1;
+int r;
+unsigned f;
+struct stat st;
+
+/* do nothing */
+if (item-attrib_mask == 0 || !item-attrib_set)
+return 0;
+
+if (lstat(path, st) == 0 
+!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
+return 0;
+}
+
+fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
+
+if (fd  0)
+return log_error_errno(errno, Cannot open \%s\: %m, path);
+
+f = item-attrib_value  item-attrib_mask;
+if (!S_ISDIR(st.st_mode))
+f = ~FS_DIRSYNC_FL;
+r = change_attr_fd(fd, item-attrib_mask, f);
+if (r  0)
+return log_error_errno(errno,
+Cannot

[systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

2015-03-10 Thread Goffredo Baroncelli 
Allow systemd-tmpfiles to set the file/directory attributes, like chattr(1)
does. Two more commands are added: 'H' and 'h' to set the attributes,
recursively and not.
---
 src/tmpfiles/tmpfiles.c | 140 
 1 file changed, 140 insertions(+)

diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
index c948d4d..165605f 100644
--- a/src/tmpfiles/tmpfiles.c
+++ b/src/tmpfiles/tmpfiles.c
@@ -40,6 +40,7 @@
 #include sys/types.h
 #include sys/param.h
 #include sys/xattr.h
+#include linux/fs.h
 
 #include log.h
 #include util.h
@@ -90,6 +91,8 @@ typedef enum ItemType {
 ADJUST_MODE = 'm', /* legacy, 'z' is identical to this */
 RELABEL_PATH = 'z',
 RECURSIVE_RELABEL_PATH = 'Z',
+SET_ATTRIB = 'h',
+RECURSIVE_SET_ATTRIB = 'H',
 } ItemType;
 
 typedef struct Item {
@@ -108,12 +111,15 @@ typedef struct Item {
 usec_t age;
 
 dev_t major_minor;
+int attrib_value;
+int attrib_mask;
 
 bool uid_set:1;
 bool gid_set:1;
 bool mode_set:1;
 bool age_set:1;
 bool mask_perms:1;
+bool attrib_set:1;
 
 bool keep_first_level:1;
 
@@ -762,6 +768,115 @@ static int path_set_acls(Item *item, const char *path) {
 return 0;
 }
 
+static int get_attrib_from_arg(Item *item) {
+static const struct { int value; char ch; } attributes[] = {
+{ FS_NOATIME_FL, 'A' },   /* do not update atime */
+{ FS_SYNC_FL, 'S' },  /* Synchronous updates */
+{ FS_DIRSYNC_FL, 'D' },   /* dirsync behaviour (directories 
only) */
+{ FS_APPEND_FL, 'a' },/* writes to file may only append */
+{ FS_COMPR_FL, 'c' }, /* Compress file */
+{ FS_NODUMP_FL, 'd' },/* do not dump file */
+{ FS_EXTENT_FL, 'e'}, /* Top of directory hierarchies*/
+{ FS_IMMUTABLE_FL, 'i' }, /* Immutable file */
+{ FS_JOURNAL_DATA_FL, 'j' }, /* Reserved for ext3 */
+{ FS_SECRM_FL, 's' }, /* Secure deletion */
+{ FS_UNRM_FL, 'u' },  /* Undelete */
+{ FS_NOTAIL_FL, 't' },/* file tail should not be merged */
+{ FS_TOPDIR_FL, 'T' },/* Top of directory hierarchies*/
+{ FS_NOCOW_FL, 'C' }, /* Do not cow file */
+{ 0, 0 }
+};
+char *p = item-argument;
+enum {
+MODE_ADD,
+MODE_DEL,
+MODE_SET
+} mode = MODE_ADD;
+unsigned long value = 0, mask = 0;
+
+if (!p) {
+log_error(\%s\: setting ATTR need an argument, item-path);
+return -EINVAL;
+}
+
+if (*p == '+') {
+mode = MODE_ADD;
+p++;
+} else if (*p == '-') {
+mode = MODE_DEL;
+p++;
+} else  if (*p == '=') {
+mode = MODE_SET;
+p++;
+}
+
+if (!*p  mode != MODE_SET) {
+log_error(\%s\: setting ATTR: argument is empty, 
item-path);
+return -EINVAL;
+}
+for (; *p ; p++) {
+int i;
+for (i = 0; attributes[i].ch  attributes[i].ch != *p; i++)
+;
+if (!attributes[i].ch) {
+log_error(\%s\: setting ATTR: unknown attr '%c', 
item-path, *p);
+return -EINVAL;
+}
+if (mode == MODE_ADD || mode == MODE_SET)
+value |= attributes[i].value;
+else
+value = ~attributes[i].value;
+mask |= attributes[i].value;
+}
+
+if (mode == MODE_SET) {
+int i;
+for (i = 0; attributes[i].ch; i++)
+mask |= attributes[i].value;
+}
+
+assert(mask);
+
+item-attrib_mask = mask;
+item-attrib_value = value;
+item-attrib_set = true;
+
+return 0;
+
+}
+
+static int path_set_attrib(Item *item, const char *path) {
+_cleanup_close_ int fd = -1;
+int r;
+unsigned f;
+struct stat st;
+
+/* do nothing */
+if (item-attrib_mask == 0 || !item-attrib_set)
+return 0;
+
+if (lstat(path, st) == 0 
+!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
+return 0;
+}
+
+fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
+
+if (fd  0)
+return log_error_errno(errno, Cannot open \%s\: %m, path);
+
+f = item-attrib_value  item-attrib_mask;
+if (!S_ISDIR(st.st_mode))
+f = ~FS_DIRSYNC_FL;
+r = change_attr_fd(fd, f, item-attrib_mask);
+if (r  0)
+return log_error_errno(errno,
+Cannot