[systemd-devel] [PATCH 1/3] Allow systemd-tmpfiles to set the file/directory attributes
Goffredo Baroncelli
kreijack at libero.it
Tue Mar 10 11:33:53 PDT 2015
On 2015-03-08 23:06, Lennart Poettering wrote:
> 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...
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
More information about the systemd-devel
mailing list