[systemd-devel] [PATCH 1/3] Allow systemd-tmpfiles to set the file/directory attributes
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Sun Mar 8 07:41:38 PDT 2015
On Sun, Mar 08, 2015 at 03:00:38PM +0100, 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[] = {
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 is called and
shouldn't this be changed to 'ite->attrib_mask = mask' etc.?
> > +
> > +
>
> 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, &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.
We also don't support compilation without O_LARGEFILE.
> > +#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.
>
> > + }
> > + r = ioctl (fd, FS_IOC_GETFLAGS, &f);
> > + if (r<0) {
> > + int save_errno = errno;
>
> There is a macro for this: PROTECT_ERRNO
>
> > + 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
>
> > + 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.
>
> Also I am not sure but everywhere in the code all function call are
> without a space before the parenthese.
>
> > + 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;
Zbyszek
More information about the systemd-devel
mailing list