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

[Lennart Poettering]

On Sun, 08.03.15 12:48, Goffredo Baroncelli (kreijack at 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 == -1) {

Not that it would matter much, but so far we checked for errors on
syscalls with "< 0" rather than "== -1"

> +                log_error_errno(errno, "Cannot open \"%s\": %m", path);
> +                return -1;

Error code.

> +        }
> +        r = ioctl (fd, FS_IOC_GETFLAGS, &f);

Weird space before (.

Also, could you please use chattr_fd() for this.

> +        if (r<0) {
> +                int save_errno = errno;
> +                log_error_errno(errno, "Cannot get attrib for \"%s\": %m", path);
> +                close (fd);
> +                errno = save_errno;
> +                return r;
> +        }

First of all, our safe_close() saves/restores errno, no need to
implement this manually. Secondly, four our own calls like this one we
return negative error codes, kernel styles. Thirdly, log_error_errno()
actually returns the error code pass in, negative, hence you can
collapse the log message printing and returning into one line. Fourth:
please use _cleanup_close_ instead of explicitly closing fds.

> +
> +        f &= ~item->attrib_mask;
> +        f |= (item->attrib_value & item->attrib_mask);
> +        if (!S_ISDIR(st.st_mode))
> +                f &= ~FS_DIRSYNC_FL;

Why is this necessary?

> +        r = ioctl (fd, FS_IOC_SETFLAGS, &f);
> +        if (r < 0) {
> +                int save_errno = errno;
> +                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;

Same as above applies here...

Lennart

-- 
Lennart Poettering, Red Hat