On 2015-03-16 04:12, Zbigniew Jędrzejewski-Szmek wrote: > 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.
I find this idea very elegant, my only concern was the memory occupation: the ascii code for the 't' letter is about 120, this means that the array will have a size of ~240 bytes... Ok that the PC have several GBs.... > >> + 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. The point is to remember to update the mask... Anyway I will add a constant with all the flag OR-ed near the flag definitions. > >> + } >> + >> + 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. Yes, it is ok, because if "path" is source of an error, the open() below will report it. I will add a comment. > > Also no braces aroudn single statemnets. ok > >> + >> + 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 Thank for the review > -- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel