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

2015-03-10 Thread Goffredo Baroncelli 
On 2015-03-08 23:06, Lennart Poettering wrote:
 On Sun, 08.03.15 12:48, Goffredo Baroncelli (kreij...@libero.it) wrote:
 
  dev_t major_minor;
 +int attrib_value;
 +int attrib_mask;
 
 int appears to be a strange choice for a bitmask. The existing
 chattr_fd() and chattr_path() calls use unsigned for this, so this
 should too...

I read the code of chattr, and in this code these values are int.
Checking the definition of the ioctls I found:

#define FS_IOC_SETFLAGS _IOW('f', 2, long)
#define FS_IOC_GETFLAGS _IOR('f', 1, long)

so the ioctl argument seems to be a *signed long value*

Anyway I changed they to unsigned long.



 
  
  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,130 @@ static int path_set_acls(Item *item, const char *path) 
 {
  return 0;
  }
  
 +static int get_attrib_from_arg(Item *item) {
 +struct attributes_list_t { int value; char ch; } ;
 
 The _t suffix is usually reserved for typedefs... 
 
 Also, it appears unnecessary to define a struct here at all, it can
 just be anonymously defined when delcaring the array.
ok
 
 +static struct attributes_list_t 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 }
 +};
 
 Indenting borked.
 
 const missing.
ok
 
 +char *p = item-argument;
 +enum { MODE_ADD, MODE_DEL, MODE_SET } mode = MODE_ADD;
 
 So far we avoided defining enums in single lines, we line-broke them
 nicely, once for each enum value. This should be done here too.
ok
 
 +int value=0, mask=0;
 
 Spaces after and before the = please.
 
 +
 +if (!p) {
 +log_error(\%s\: setting ATTR need an argument, 
 item-path);
 +return -1;
 
 Please use explicit error codes, like -EINVAL, don't make up numeric
 error codes like -1.
ok
 
 Also see CODING_STYLE
 
 +}
 +
 +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 -4;
 
 Error code
ok
 
 +}
 +for ( ; *p ; p++ ) {
 
 Weird spaces...
ok
 
 +int i;
 +for ( i = 0; attributes[i].ch  attributes[i].ch != *p 
 ;i++);
 
 Weird spaces...
 
 Also, please add an explicit line break before the ;, so that it is
 clear that this is a for loop without a body.
ok

 +if (!attributes[i].ch) {
 +log_error(\%s\: setting ATTR: unknown attr '%c',
 +item-path, *p);
 
 We don't break lines this eagerly in systemd.
ok
 
 +return -2;
 
 Error code...
 
 +}
 +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++)
 
 Weird spaces...
ok
 
 +
 +static int path_set_attrib(Item *item, const char *path) {
 +int fd, r, f;
 +struct stat st;
 +
 +/* do nothing */
 +if (item-attrib_mask == 0 || !item-attrib_set)
 +return 0;
 +
 +if (!lstat(path, st) 
 +!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
 +return 0;
 +}

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

2015-03-09 Thread Ronny Chevalier 
2015-03-10 0:17 GMT+01:00 Goffredo Baroncelli kreij...@libero.it:
 On 2015-03-08 15:00, Ronny Chevalier wrote:
 2015-03-08 12:48 GMT+01:00 Goffredo Baroncelli kreij...@libero.it:
 From: Goffredo Baroncelli kreij...@inwind.it


 Hi,

 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.

 Signed-off-by: Goffredo Baroncelli kreij...@inwind.it

 No Signed-off-by in systemd.

 ---
  src/tmpfiles/tmpfiles.c | 155 
 
  1 file changed, 155 insertions(+)

 diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
 index c948d4d..cb77047 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,130 @@ static int path_set_acls(Item *item, const char 
 *path) {
  return 0;
  }

 +static int get_attrib_from_arg(Item *item) {
 +struct attributes_list_t { int value; char ch; } ;
 +static struct attributes_list_t 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;
 +int value=0, mask=0;
 +
 +if (!p) {
 +log_error(\%s\: setting ATTR need an argument, 
 item-path);
 +return -1;

 Please use errno style error code. In this case -EINVAL is appropriate.
 ok

 +}
 +
 +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 -4;

 Same here.
 ok

 +}
 +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 -2;

 Same here.
 ok

 +}
 +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 = ~mask;
 +item-attrib_value |= value;
 +item-attrib_set = true;
 +
 +

 Useless newline.
 ok

 +return 0;
 +
 +}
 +
 +static int path_set_attrib(Item *item, const char *path) {
 +int fd, r, f;
 +struct stat st;
 +
 +

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

2015-03-09 Thread Goffredo Baroncelli 
On 2015-03-08 15:00, Ronny Chevalier wrote:
 2015-03-08 12:48 GMT+01:00 Goffredo Baroncelli kreij...@libero.it:
 From: Goffredo Baroncelli kreij...@inwind.it

 
 Hi,
 
 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.

 Signed-off-by: Goffredo Baroncelli kreij...@inwind.it
 
 No Signed-off-by in systemd.
 
 ---
  src/tmpfiles/tmpfiles.c | 155 
 
  1 file changed, 155 insertions(+)

 diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
 index c948d4d..cb77047 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,130 @@ static int path_set_acls(Item *item, const char *path) 
 {
  return 0;
  }

 +static int get_attrib_from_arg(Item *item) {
 +struct attributes_list_t { int value; char ch; } ;
 +static struct attributes_list_t 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;
 +int value=0, mask=0;
 +
 +if (!p) {
 +log_error(\%s\: setting ATTR need an argument, 
 item-path);
 +return -1;
 
 Please use errno style error code. In this case -EINVAL is appropriate.
ok
 
 +}
 +
 +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 -4;
 
 Same here.
ok
 
 +}
 +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 -2;
 
 Same here.
ok
 
 +}
 +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 = ~mask;
 +item-attrib_value |= value;
 +item-attrib_set = true;
 +
 +
 
 Useless newline.
ok
 
 +return 0;
 +
 +}
 +
 +static int path_set_attrib(Item *item, const char *path) {
 +int fd, r, f;
 +struct stat st;
 +
 +/* do nothing */
 +if (item-attrib_mask == 0 ||

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

2015-03-08 Thread Lennart Poettering 
On Sun, 08.03.15 12:48, Goffredo Baroncelli (kreij...@libero.it) wrote:

  dev_t major_minor;
 +int attrib_value;
 +int attrib_mask;

int appears to be a strange choice for a bitmask. The existing
chattr_fd() and chattr_path() calls use unsigned for this, so this
should too...

  
  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,130 @@ static int path_set_acls(Item *item, const char *path) {
  return 0;
  }
  
 +static int get_attrib_from_arg(Item *item) {
 +struct attributes_list_t { int value; char ch; } ;

The _t suffix is usually reserved for typedefs... 

Also, it appears unnecessary to define a struct here at all, it can
just be anonymously defined when delcaring the array.

 +static struct attributes_list_t 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 }
 +};

Indenting borked.

const missing.

 +char *p = item-argument;
 +enum { MODE_ADD, MODE_DEL, MODE_SET } mode = MODE_ADD;

So far we avoided defining enums in single lines, we line-broke them
nicely, once for each enum value. This should be done here too.

 +int value=0, mask=0;

Spaces after and before the = please.

 +
 +if (!p) {
 +log_error(\%s\: setting ATTR need an argument, 
 item-path);
 +return -1;

Please use explicit error codes, like -EINVAL, don't make up numeric
error codes like -1.

Also see CODING_STYLE

 +}
 +
 +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 -4;

Error code

 +}
 +for ( ; *p ; p++ ) {

Weird spaces...

 +int i;
 +for ( i = 0; attributes[i].ch  attributes[i].ch != *p 
 ;i++);

Weird spaces...

Also, please add an explicit line break before the ;, so that it is
clear that this is a for loop without a body.

 +if (!attributes[i].ch) {
 +log_error(\%s\: setting ATTR: unknown attr '%c',
 +item-path, *p);

We don't break lines this eagerly in systemd.

 +return -2;

Error code...

 +}
 +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++)

Weird spaces...

 +
 +static int path_set_attrib(Item *item, const char *path) {
 +int fd, r, f;
 +struct stat st;
 +
 +/* do nothing */
 +if (item-attrib_mask == 0 || !item-attrib_set)
 +return 0;
 +
 +if (!lstat(path, st) 
 +!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
 +return 0;
 +}

Please avoid using boolean checks for numeric values. 

But more importantly, why check this at all? Sounds completely Ok to
apply this to anything

 +#ifdef O_LARGEFILE
 +fd = open (path, O_RDONLY|O_NONBLOCK|O_LARGEFILE);
 +#else
 +fd = open (path, O_RDONLY|O_NONBLOCK);
 +#endif

systemd sets AC_SYS_LARGEFILE, we do not support builds with 32bit
file offsets. Hence, please do not use O_LARGEFILE, since it is always
implied.

 +if (fd ==

[systemd-devel] [PATCH 1/3] Allow systemd-tmpfiles to set the file/directory attributes

2015-03-08 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.

Signed-off-by: Goffredo Baroncelli kreij...@inwind.it
---
 src/tmpfiles/tmpfiles.c | 155 
 1 file changed, 155 insertions(+)

diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
index c948d4d..cb77047 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,130 @@ static int path_set_acls(Item *item, const char *path) {
 return 0;
 }
 
+static int get_attrib_from_arg(Item *item) {
+struct attributes_list_t { int value; char ch; } ;
+static struct attributes_list_t 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;
+int value=0, mask=0;
+
+if (!p) {
+log_error(\%s\: setting ATTR need an argument, item-path);
+return -1;
+}
+
+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 -4;
+}
+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 -2;
+}
+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 = ~mask;
+item-attrib_value |= value;
+item-attrib_set = true;
+
+
+return 0;
+
+}
+
+static int path_set_attrib(Item *item, const char *path) {
+int fd, r, f;
+struct stat st;
+
+/* do nothing */
+if (item-attrib_mask == 0 || !item-attrib_set)
+return 0;
+
+if (!lstat(path, st) 
+!S_ISREG(st.st_mode)  !S_ISDIR(st.st_mode)) {
+return 0;
+}
+#ifdef O_LARGEFILE
+fd = open (path, O_RDONLY|O_NONBLOCK|O_LARGEFILE);
+#else
+fd = open (path, O_RDONLY|O_NONBLOCK);
+#endif
+if (fd == -1) {
+log_error_errno(errno, Cannot open \%s\: %m, path);
+

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

2015-03-08 Thread Ronny Chevalier 
2015-03-08 12:48 GMT+01:00 Goffredo Baroncelli kreij...@libero.it:
 From: Goffredo Baroncelli kreij...@inwind.it


Hi,

 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.

 Signed-off-by: Goffredo Baroncelli kreij...@inwind.it

No Signed-off-by in systemd.

 ---
  src/tmpfiles/tmpfiles.c | 155 
 
  1 file changed, 155 insertions(+)

 diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
 index c948d4d..cb77047 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,130 @@ static int path_set_acls(Item *item, const char *path) {
  return 0;
  }

 +static int get_attrib_from_arg(Item *item) {
 +struct attributes_list_t { int value; char ch; } ;
 +static struct attributes_list_t 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;
 +int value=0, mask=0;
 +
 +if (!p) {
 +log_error(\%s\: setting ATTR need an argument, 
 item-path);
 +return -1;

Please use errno style error code. In this case -EINVAL is appropriate.

 +}
 +
 +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 -4;

Same here.

 +}
 +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 -2;

Same here.

 +}
 +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 = ~mask;
 +item-attrib_value |= value;
 +item-attrib_set = true;
 +
 +

Useless newline.

 +return 0;
 +
 +}
 +
 +static int path_set_attrib(Item *item, const char *path) {
 +int fd, r, f;
 +struct stat st;
 +
 +/* do nothing */
 +if (item-attrib_mask == 0 || !item-attrib_set)
 +return 0;
 +
 +if (!lstat(path,

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

2015-03-08 Thread Zbigniew Jędrzejewski-Szmek 
On Sun, Mar 08, 2015 at 03:00:38PM +0100, Ronny Chevalier wrote:
 2015-03-08 12:48 GMT+01:00 Goffredo Baroncelli kreij...@libero.it:
  From: Goffredo Baroncelli kreij...@inwind.it
 
 
 Hi,
 
  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.
 
  Signed-off-by: Goffredo Baroncelli kreij...@inwind.it
 
 No Signed-off-by in systemd.
 
  ---
   src/tmpfiles/tmpfiles.c | 155 
  
   1 file changed, 155 insertions(+)
 
  diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
  index c948d4d..cb77047 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,130 @@ static int path_set_acls(Item *item, const char 
  *path) {
   return 0;
   }
 
  +static int get_attrib_from_arg(Item *item) {
  +struct attributes_list_t { int value; char ch; } ;
  +static struct attributes_list_t attributes[] = {
Also const.

  +{ FS_NOATIME_FL, 'A' },   /* do not update atime */
This indentation is excessive.

  +{ 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;
  +int value=0, mask=0;
  +
  +if (!p) {
  +log_error(\%s\: setting ATTR need an argument, 
  item-path);
  +return -1;
 
 Please use errno style error code. In this case -EINVAL is appropriate.
 
  +}
  +
  +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 -4;
 
 Same here.
 
  +}
  +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 -2;
 
 Same here.
 
  +}
  +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 = ~mask;
  +item-attrib_value |= value;
  +item-attrib_set = true;
Isn't the item totally new before this function