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

Ronny Chevalier chevalier.ronny at gmail.com
Mon Mar 9 16:20:04 PDT 2015


2015-03-10 0:17 GMT+01:00 Goffredo Baroncelli <kreijack at libero.it>:
> On 2015-03-08 15:00, Ronny Chevalier wrote:
>> 2015-03-08 12:48 GMT+01:00 Goffredo Baroncelli <kreijack at libero.it>:
>>> From: Goffredo Baroncelli <kreijack at 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 <kreijack at 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 || !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);
>>
>> You should add a O_CLOEXEC.
> Why ? Am not against your suggestion, but I am not able the understand the reason: this code doesn't fork,
> so O_CLOEXEC is not useful.

Sure, just an habit in the CODING_STYLE to ensure that no fd leaks.

>
>>
>>> +#endif
>>> +        if (fd == -1) {
>>> +                log_error_errno(errno, "Cannot open \"%s\": %m", path);
>>> +                return -1;
>>
>> return log_error_errno(...);
>>
>> It returns the errno code provided.
> ok
>>
>>> +        }
>>> +        r = ioctl (fd, FS_IOC_GETFLAGS, &f);
>>> +        if (r<0) {
>>> +                int save_errno = errno;
>>
>> There is a macro for this: PROTECT_ERRNO
> what nice macro !
>>
>>> +                log_error_errno(errno, "Cannot get attrib for \"%s\": %m", path);
>>> +                close (fd);
>>> +                errno = save_errno;
>>> +                return r;
>>> +        }
>>> +
>>> +        f &= ~item->attrib_mask;
>>> +        f |= (item->attrib_value & item->attrib_mask);
>>> +        if (!S_ISDIR(st.st_mode))
>>> +                f &= ~FS_DIRSYNC_FL;
>>> +        r = ioctl (fd, FS_IOC_SETFLAGS, &f);
>>> +        if (r < 0) {
>>> +                int save_errno = errno;
>>
>> Same here, PROTECT_ERRNO
> ok
>>
>>> +                log_error_errno(errno,
>>> +                        "Cannot set attrib for \"%s\", value=0x%08x, mask=0x%08x: %m",
>>> +                        path, item->attrib_value, item->attrib_mask);
>>> +                close (fd);
>>> +                errno = save_errno;
>>> +                return r;
>>> +        }
>>> +        close (fd);
>>
>> You can use _cleanup_close_ for this fd.
> Unfortunately no; I exit from the function before opening the fd...

_cleanup_close_ int fd = -1;

The close will be a nop.

>>
>> Also I am not sure but everywhere in the code all function call are
>> without a space before the parenthese.
> I correct it
>>
>>> +        return 0;
>>> +}
>>> +
>>>  static int write_one_file(Item *i, const char *path) {
>>>          _cleanup_close_ int fd = -1;
>>>          int flags, r = 0;
>>> @@ -1203,6 +1333,18 @@ static int create_item(Item *i) {
>>>                  if (r < 0)
>>>                          return r;
>>>                  break;
>>> +
>>> +        case SET_ATTRIB:
>>> +                r = glob_item(i, path_set_attrib, false);
>>> +                if (r < 0)
>>> +                        return r;
>>> +                break;
>>> +
>>> +        case RECURSIVE_SET_ATTRIB:
>>> +                r = glob_item(i, path_set_attrib, true);
>>> +                if (r < 0)
>>> +                        return r;
>>> +                break;
>>>          }
>>>
>>>          log_debug("%s created successfully.", i->path);
>>> @@ -1269,6 +1411,8 @@ static int remove_item(Item *i) {
>>>          case RECURSIVE_SET_XATTR:
>>>          case SET_ACL:
>>>          case RECURSIVE_SET_ACL:
>>> +        case SET_ATTRIB:
>>> +        case RECURSIVE_SET_ATTRIB:
>>>                  break;
>>>
>>>          case REMOVE_PATH:
>>> @@ -1653,6 +1797,17 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) {
>>>                          return r;
>>>                  break;
>>>
>>> +        case SET_ATTRIB:
>>> +        case RECURSIVE_SET_ATTRIB:
>>> +                if (!i.argument) {
>>> +                        log_error("[%s:%u] Set attrib requires argument.", fname, line);
>>> +                        return -EBADMSG;
>>> +                }
>>> +                r = get_attrib_from_arg(&i);
>>> +                if (r < 0)
>>> +                        return r;
>>> +                break;
>>> +
>>>          default:
>>>                  log_error("[%s:%u] Unknown command type '%c'.", fname, line, (char) i.type);
>>>                  return -EBADMSG;
>>> --
>>> 2.1.4
>>>
>>> _______________________________________________
>>> systemd-devel mailing list
>>> systemd-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>>
>
>
> --
> gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
> Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5


More information about the systemd-devel mailing list