[systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes
Goffredo Baroncelli
kreijack at libero.it
Mon Mar 16 11:49:20 PDT 2015
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
More information about the systemd-devel
mailing list