On 2015-03-08 23:06, Lennart Poettering wrote: > On Sun, 08.03.15 12:48, Goffredo Baroncelli (kreij...@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...
I read the code of chattr, and in this code these values are int. Checking the definition of the ioctls I found: #define FS_IOC_SETFLAGS _IOW('f', 2, long) #define FS_IOC_GETFLAGS _IOR('f', 1, long) so the ioctl argument seems to be a *signed long value*.... Anyway I changed they to unsigned long. > >> >> 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. ok > >> + 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. ok > >> + 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. ok > >> + 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. ok > > 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.... ok > >> + } >> + for ( ; *p ; p++ ) { > > Weird spaces... ok > >> + 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. ok >> + if (!attributes[i].ch) { >> + log_error("\"%s\": setting ATTR: unknown attr '%c'", >> + item->path, *p); > > We don't break lines this eagerly in systemd. ok > >> + 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... ok > >> + >> +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.... I found these check in the source code of chattr. As conservative approach I copied it. It seems that the chattr author limited to file/directories the changing of attributes. > >> +#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. ok > >> + if (fd == -1) { > > Not that it would matter much, but so far we checked for errors on > syscalls with "< 0" rather than "== -1" ok, > >> + log_error_errno(errno, "Cannot open \"%s\": %m", path); >> + return -1; > > Error code. ok > >> + } >> + r = ioctl (fd, FS_IOC_GETFLAGS, &f); > > Weird space before (. > > Also, could you please use chattr_fd() for this. If so I have to call two time chattr_fd(): something like chattr_fd(fd, true, values|mask); chattr_fd(fd, false, ~mask); I prefer to add to shared/util.c another function like int change_attr_fd(fd, unsigned long mask, unsigned long value).... what do you think ? > >> + 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. Ok, > >> + >> + 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? Is a check copied from chattr; FS_DIRSYNC_FL is meaningful only for directory, so it make sense to reset it if the entry is not a directory. For example if the .conf file contains H /path/bla/bla - - - - +D without the check even the file would have FS_DIRSYNC_FL set. > >> + 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 > -- 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