On Mon, 16.03.15 20:33, Goffredo Baroncelli (kreij...@libero.it) wrote:
Sorry for warming up this old thread, but a few issues I see with this?
> 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;
I thought this was unsigned now?
>
> +static int get_attrib_from_arg(Item *item) {
> + static const unsigned attributes[] = {
> + [(uint8_t)'A'] = FS_NOATIME_FL, /* do not update atime */
> + [(uint8_t)'S'] = FS_SYNC_FL, /* Synchronous updates */
> + [(uint8_t)'D'] = FS_DIRSYNC_FL, /* dirsync behaviour
> (directories only) */
> + [(uint8_t)'a'] = FS_APPEND_FL, /* writes to file may only
> append */
> + [(uint8_t)'c'] = FS_COMPR_FL, /* Compress file */
> + [(uint8_t)'d'] = FS_NODUMP_FL, /* do not dump file */
> + [(uint8_t)'e'] = FS_EXTENT_FL, /* Top of directory
> hierarchies*/
> + [(uint8_t)'i'] = FS_IMMUTABLE_FL, /* Immutable file */
> + [(uint8_t)'j'] = FS_JOURNAL_DATA_FL, /* Reserved for ext3 */
> + [(uint8_t)'s'] = FS_SECRM_FL, /* Secure deletion */
> + [(uint8_t)'u'] = FS_UNRM_FL, /* Undelete */
> + [(uint8_t)'t'] = FS_NOTAIL_FL, /* file tail should not be
> merged */
> + [(uint8_t)'T'] = FS_TOPDIR_FL, /* Top of directory
> hierarchies*/
> + [(uint8_t)'C'] = FS_NOCOW_FL, /* Do not cow file */
> + };
Hmm, that's quite a sparse array! It wastes 116 entries, just to
store 13 entries... I think this should really be an array fo structs
with the chars and their flag, which we iterate through...
> + const unsigned all_flags = FS_NOATIME_FL|FS_SYNC_FL|FS_DIRSYNC_FL|
> + FS_APPEND_FL|FS_COMPR_FL|FS_NODUMP_FL|FS_EXTENT_FL|
> + FS_IMMUTABLE_FL|FS_JOURNAL_DATA_FL|FS_SECRM_FL|FS_UNRM_FL|
> + FS_NOTAIL_FL|FS_TOPDIR_FL|FS_NOCOW_FL;
> + char *p = item->argument;
> + enum {
> + MODE_ADD,
> + MODE_DEL,
> + MODE_SET
> + } mode = MODE_ADD;
> + unsigned long value = 0, mask = 0;
And now it is "unsigned long", not just "unsigned anymore?
> +
> +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;
> + /*
> + * It is OK to ignore an lstat() error, because the error
> + * will be catch by the open() below anyway
> + */
> + if (lstat(path, &st) == 0 &&
> + !S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) {
> + return 0;
> + }
Why do we need this check? What happens if we apply this onto
a device node, or socket, or whatever?
I really don#t like this seperate lstat() + open(). If it all it
should be open() + fstat(), to avoid TTOCTOU issues...
> + fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
> +
> + if (fd < 0)
> + return log_error_errno(errno, "Cannot open \"%s\": %m",
> path);
>
> + case SET_ATTRIB:
> + case RECURSIVE_SET_ATTRIB:
> + if (!i.argument) {
> + log_error("[%s:%u] Set attrib requires
> argument.", fname, line);
Please don't abbreviate words like "attrib" in human language...
Lennart
--
Lennart Poettering, Red Hat
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
