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

[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 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;
> +        }
Is it OK to ignore lstat error? If yes, please add a comment why.

Also no braces aroudn single statemnets.

> +
> +        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 set attrib for \"%s\", value=0x%08x, mask=0x%08x: %m",
> +                        path, item->attrib_value, item->attrib_mask);
> +
> +        return 0;
> +}
> +

Zbyszek