Discussion:
[PATCH 1/4] Add change_attr_fd()
(too old to reply)
Goffredo Baroncelli
2015-03-16 19:33:49 UTC
Permalink
From: Goffredo Baroncelli <***@inwind.it>

Add change_attr_fd() function to modify the file/directory attribute.
---
src/shared/util.c | 22 ++++++++++++++++++++++
src/shared/util.h | 1 +
2 files changed, 23 insertions(+)

diff --git a/src/shared/util.c b/src/shared/util.c
index ba035ca..56097ec 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -7832,6 +7832,28 @@ int chattr_path(const char *p, bool b, unsigned mask) {
return chattr_fd(fd, b, mask);
}

+int change_attr_fd(int fd, unsigned value, unsigned mask) {
+ unsigned old_attr, new_attr;
+
+ assert(fd >= 0);
+
+ if (mask == 0)
+ return 0;
+
+ if (ioctl(fd, FS_IOC_GETFLAGS, &old_attr) < 0)
+ return -errno;
+
+ new_attr = (old_attr & ~mask) |(value & mask);
+
+ if (new_attr == old_attr)
+ return 0;
+
+ if (ioctl(fd, FS_IOC_SETFLAGS, &new_attr) < 0)
+ return -errno;
+
+ return 0;
+}
+
int read_attr_fd(int fd, unsigned *ret) {
assert(fd >= 0);

diff --git a/src/shared/util.h b/src/shared/util.h
index a83b588..a0bc883 100644
--- a/src/shared/util.h
+++ b/src/shared/util.h
@@ -1052,6 +1052,7 @@ int same_fd(int a, int b);

int chattr_fd(int fd, bool b, unsigned mask);
int chattr_path(const char *p, bool b, unsigned mask);
+int change_attr_fd(int fd, unsigned value, unsigned mask);

int read_attr_fd(int fd, unsigned *ret);
int read_attr_path(const char *p, unsigned *ret);
--
2.1.4
Goffredo Baroncelli
2015-03-16 19:33:51 UTC
Permalink
From: Goffredo Baroncelli <***@inwind.it>

Update the man page of tmpfiles.d(5), to document the new h/H command.
---
man/tmpfiles.d.xml | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)

diff --git a/man/tmpfiles.d.xml b/man/tmpfiles.d.xml
index 8815bf9..a532f91 100644
--- a/man/tmpfiles.d.xml
+++ b/man/tmpfiles.d.xml
@@ -303,6 +303,41 @@
</varlistentry>

<varlistentry>
+ <term><varname>h</varname></term>
+ <listitem><para>Set file/directory attributes. Lines of this type
+ accept shell-style globs in place of normal path names.</para>
+
+ <para>The format of the argument field is <varname>[+-=][aAcCdDeijsStTu]
+ </varname></para>
+
+ <para>The prefix <varname>+</varname> (the default one) causes the
+ attribute(s) to be added; <varname>-</varname> causes the
+ attribute(s) to be removed; <varname>=</varname>
+ causes the attributes to set exactly as the following letters.</para>
+ <para>The letters <literal>aAcCdDeijsStTu</literal> select the new
+ attributes for the files, see
+ <citerefentry><refentrytitle>chattr</refentrytitle>
+ <manvolnum>1</manvolnum></citerefentry> for further information.
+ </para>
+ <para>Passing only <varname>=</varname> as argument,
+ resets all the file attributes listed above. It has to be pointed
+ out that the <varname>=</varname> prefix, limits itself to the
+ attributes corresponding to the letters listed here. All other
+ attributes will be left untouched.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><varname>H</varname></term>
+ <listitem><para>Recursively set file/directory attributes. Lines
+ of this type accept shell-style globs in place of normal
+ path names.
+ </para></listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><varname>a</varname></term>
<term><varname>a+</varname></term>
<listitem><para>Set POSIX ACLs (access control lists). If
@@ -529,6 +564,7 @@
<citerefentry project='man-pages'><refentrytitle>setfattr</refentrytitle><manvolnum>1</manvolnum></citerefentry>,
<citerefentry project='man-pages'><refentrytitle>setfacl</refentrytitle><manvolnum>1</manvolnum></citerefentry>,
<citerefentry project='man-pages'><refentrytitle>getfacl</refentrytitle><manvolnum>1</manvolnum></citerefentry>
+ <citerefentry project='man-pages'><refentrytitle>chattr</refentrytitle><manvolnum>1</manvolnum></citerefentry>
</para>
</refsect1>
--
2.1.4
Goffredo Baroncelli
2015-03-16 19:33:50 UTC
Permalink
From: Goffredo Baroncelli <***@inwind.it>

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..e1a22f5 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 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 */
+ };
+ 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;
+
+ 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++) {
+ if ((uint8_t)*p > ELEMENTSOF(attributes) || attributes[(uint8_t)*p] == 0) {
+ log_error("\"%s\": setting ATTR: unknown attr '%c'", item->path, *p);
+ return -EINVAL;
+ }
+ if (mode == MODE_ADD || mode == MODE_SET)
+ value |= attributes[(uint8_t)*p];
+ else
+ value &= ~attributes[(uint8_t)*p];
+ mask |= attributes[(uint8_t)*p];
+ }
+
+ if (mode == MODE_SET)
+ mask |= all_flags;
+
+ 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;
+ /*
+ * 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;
+ }
+
+ 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;
+}
+
static int write_one_file(Item *i, const char *path) {
_cleanup_close_ int fd = -1;
int flags, r = 0;
@@ -1203,6 +1318,18 @@ static int create_item(Item *i) {
if (r < 0)
return r;
break;
+
+ case SET_ATTRIB:
+ r = glob_item(i, path_set_attrib, false);
+ if (r < 0)
+ return r;
+ break;
+
+ case RECURSIVE_SET_ATTRIB:
+ r = glob_item(i, path_set_attrib, true);
+ if (r < 0)
+ return r;
+ break;
}

log_debug("%s created successfully.", i->path);
@@ -1269,6 +1396,8 @@ static int remove_item(Item *i) {
case RECURSIVE_SET_XATTR:
case SET_ACL:
case RECURSIVE_SET_ACL:
+ case SET_ATTRIB:
+ case RECURSIVE_SET_ATTRIB:
break;

case REMOVE_PATH:
@@ -1653,6 +1782,17 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) {
return r;
break;

+ case SET_ATTRIB:
+ case RECURSIVE_SET_ATTRIB:
+ if (!i.argument) {
+ log_error("[%s:%u] Set attrib requires argument.", fname, line);
+ return -EBADMSG;
+ }
+ r = get_attrib_from_arg(&i);
+ if (r < 0)
+ return r;
+ break;
+
default:
log_error("[%s:%u] Unknown command type '%c'.", fname, line, (char) i.type);
return -EBADMSG;
--
2.1.4
Lennart Poettering
2015-04-08 18:36:12 UTC
Permalink
On Mon, 16.03.15 20:33, Goffredo Baroncelli (***@libero.it) wrote:

Sorry for warming up this old thread, but a few issues I see with this?
Post by Goffredo Baroncelli
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?
Post by Goffredo Baroncelli
+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...
Post by Goffredo Baroncelli
+ 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?
Post by Goffredo Baroncelli
+
+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...
Post by Goffredo Baroncelli
+ fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
+
+ if (fd < 0)
+ return log_error_errno(errno, "Cannot open \"%s\": %m", path);
+ 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
Goffredo Baroncelli
2015-04-08 19:21:13 UTC
Permalink
Post by Lennart Poettering
Sorry for warming up this old thread, but a few issues I see with this?
Post by Goffredo Baroncelli
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?
Sew below
Post by Lennart Poettering
Post by Goffredo Baroncelli
+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...
I accepted a suggestion of Zbyszek; this solution avoids to loop over the list...
Post by Lennart Poettering
Post by Goffredo Baroncelli
+ 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?
For 32 bit, the ioctl FS_IOC_GETFLAGS seems to be defined with an "int" as argument; for 64bit, the argument is a long; however chattr_fd() uses unsigned... Yes this is a mess; probably "int" is a good answer
Post by Lennart Poettering
Post by Goffredo Baroncelli
+
+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 copied this check from the source of chattr; the reason is:

(from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=152029#10)
[...]
Post by Lennart Poettering
$ lsattr /dev/dri/card0
s-S----------- /dev/dri/card0
Segmentation fault (core dumped)
$ ls -al /dev/dri/card0
crw-rw-rw- 1 root root 226, 0 jun 30 11:12 /dev/dri/card0
lsattr and chattr can not work against device files, since they use
ioctl's which are supported by the ext2 filesystem, but which aren't
supported by normal devices (and unfortunately normal devices don't
forwawrd the ioctl on to the ext2 filesystem). In this case, the ioctl
used by lsattr is apparently also implemented by the DRI device driver,
but probably does something completely different. So the returned
device structure contains garbage, which causes lsattr to core dump
[...]
Post by Lennart Poettering
I really don#t like this seperate lstat() + open(). If it all it
should be open() + fstat(), to avoid TTOCTOU issues...
Post by Goffredo Baroncelli
+ fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
+
+ if (fd < 0)
+ return log_error_errno(errno, "Cannot open \"%s\": %m", path);
+ if (!i.argument) {
+ log_error("[%s:%u] Set attrib requires argument.", fname, line);
Please don't abbreviate words like "attrib" in human language...
Lennart
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Lennart Poettering
2015-04-08 20:59:37 UTC
Permalink
Post by Goffredo Baroncelli
Post by Lennart Poettering
Post by Goffredo Baroncelli
+ 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?
For 32 bit, the ioctl FS_IOC_GETFLAGS seems to be defined with an
"int" as argument; for 64bit, the argument is a long; however
chattr_fd() uses unsigned... Yes this is a mess; probably "int" is a
good answer
Well, the kernel is a bit confused, but as far as I can see it
generally treats it as "unsigned". Only in the ext234 code there's
this gem that it calls get_user() and pretends it was an int, while
reading it into an unsigned. btrfs never falls for that, and neither
do most other file systems afaics.

e2fsprogs is also confused. It treats it mostly as "unsigned long",
but then converts it to "int" right before issueing the ioctl.

I guess the kernel devs haven't understood the concept of "type
safety" yet, or even worse, of "types" ;-)

But anway, I'd stay close to the kernel, so let's make this an
"unsigned".

Flag fields that are signed would be weird anyway...
Post by Goffredo Baroncelli
Post by Lennart Poettering
Post by Goffredo Baroncelli
+ 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?
(from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=152029#10)
[...]
Post by Lennart Poettering
$ lsattr /dev/dri/card0
s-S----------- /dev/dri/card0
Segmentation fault (core dumped)
$ ls -al /dev/dri/card0
crw-rw-rw- 1 root root 226, 0 jun 30 11:12 /dev/dri/card0
lsattr and chattr can not work against device files, since they use
ioctl's which are supported by the ext2 filesystem, but which aren't
supported by normal devices (and unfortunately normal devices don't
forwawrd the ioctl on to the ext2 filesystem). In this case, the ioctl
used by lsattr is apparently also implemented by the DRI device driver,
but probably does something completely different. So the returned
device structure contains garbage, which causes lsattr to core dump
[...]
I see, this is indeed a real problem (and in fact we might need to fix
this for a couple of other fs related ioctls, too).

Either way, this really should be an fstat() on the opened file and
not a stat() before, due to TOCTTOU.

Anyway, I now made the necessary changes, and changed a few other
things too, and gave it some testing. Please have a look if it still
looks good to you!

Lennart
--
Lennart Poettering, Red Hat
Goffredo Baroncelli
2015-04-12 20:19:56 UTC
Permalink
Hi Lennart,
Post by Lennart Poettering
+
Post by Goffredo Baroncelli
+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...
I am checking your changes; you modified the code above in :

fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
if (fd < 0)
return log_error_errno(errno, "Cannot open '%s': %m", path);

if (fstat(fd, &st) < 0)
return log_error_errno(errno, "Cannot stat '%s': %m", path);

/* Issuing the file attribute ioctls on device nodes is not
* safe, as that will be delivered to the drivers, not the
* file system containing the device node. */
if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) {
log_error("Setting file flags is only supported on regular files and directories, cannot set on '%s'.", path);
return -EINVAL;
}


However the original code catch also the case where the file is a soft-link.
The same check is performed also by chattr(1); I suggest to leave the original
behavior, changing

fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
in
fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_NOFOLLOW);

and checking if the errno is ELOOP. In this case a further check is performed to
verify if the file is a link or the error is due to a too many symbolic link.
Then an appropriate message error is printed.

What do you think ?

Goffredo
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Lennart Poettering
2015-04-13 09:31:07 UTC
Permalink
Post by Goffredo Baroncelli
However the original code catch also the case where the file is a soft-link.
The same check is performed also by chattr(1); I suggest to leave the original
behavior, changing
fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
in
fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_NOFOLLOW);
and checking if the errno is ELOOP. In this case a further check is performed to
verify if the file is a link or the error is due to a too many symbolic link.
Then an appropriate message error is printed.
What do you think ?
We should probably either follow symlinks for all of tmpfiles'
operations or for none.

While I generally believe that we probably shouldn't follow symlinks,
it's really difficult to implement given that fchmodat() currenlty
doesn't work with AT_SYMLINK_FOLLOW (according to the man page at
least), and acl_set_file doesn't allow not following symlinks either... :-(

Hmm, I can't say I like this I must say.

ideas?

Lennart
--
Lennart Poettering, Red Hat
Lennart Poettering
2015-04-13 13:29:44 UTC
Permalink
Post by Lennart Poettering
Post by Goffredo Baroncelli
However the original code catch also the case where the file is a soft-link.
The same check is performed also by chattr(1); I suggest to leave the original
behavior, changing
fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
in
fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_NOFOLLOW);
and checking if the errno is ELOOP. In this case a further check is performed to
verify if the file is a link or the error is due to a too many symbolic link.
Then an appropriate message error is printed.
What do you think ?
We should probably either follow symlinks for all of tmpfiles'
operations or for none.
While I generally believe that we probably shouldn't follow symlinks,
it's really difficult to implement given that fchmodat() currenlty
doesn't work with AT_SYMLINK_FOLLOW (according to the man page at
least), and acl_set_file doesn't allow not following symlinks either... :-(
Hmm, I can't say I like this I must say.
ideas?
I now fixed much of the code to not follow symlinks anymore. With a
combination of O_PATH and using /proc/self/fd/%i for acl_set_file() I
managed to get all of the xattr, acl, file attribute, chmod, chown
code to not follow symlinks anymore.

I also documented this behaviour in the man page for the lines where
this applies.

I am pretty sure we should not follow symlinks when creating new file
objects or adjusting existing ones (with the notable exception of "w"
lines). Right now we still follow symlinks when creating dirs, fifos,
device nodes. We should fix that too.

Anyway, Goffredo, the file attribute code will not follow symlinks
anymore, hence this should settle this issue you raised.

Lennart
--
Lennart Poettering, Red Hat
Goffredo Baroncelli
2015-03-16 19:33:52 UTC
Permalink
From: Goffredo Baroncelli <***@inwind.it>

Add a new tmpfiles.d snippets to set the NOCOW attributes for the
journal files. This allow better perfomance when the root file system
is BTRFS. Pay attention that the NOCOW flags disables the checksum and
prevent scrub to rebuild a corrupted journal.
---
tmpfiles.d/journal-nocow.conf | 12 ++++++++++++
1 file changed, 12 insertions(+)
create mode 100644 tmpfiles.d/journal-nocow.conf

diff --git a/tmpfiles.d/journal-nocow.conf b/tmpfiles.d/journal-nocow.conf
new file mode 100644
index 0000000..43a4f2b
--- /dev/null
+++ b/tmpfiles.d/journal-nocow.conf
@@ -0,0 +1,12 @@
+# This file is part of systemd.
+#
+# systemd is free software; you can redistribute it and/or modify it
+# under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation; either version 2.1 of the License, or
+# (at your option) any later version.
+
+# See tmpfiles.d(5) for details
+
+
+# set the journal file as NOCOW; only valid for BTRFS filesystem
+h /var/log/journal/%m - - - - +C
--
2.1.4
Zbigniew Jędrzejewski-Szmek
2015-03-21 13:37:31 UTC
Permalink
Post by Goffredo Baroncelli
Add a new tmpfiles.d snippets to set the NOCOW attributes for the
journal files. This allow better perfomance when the root file system
is BTRFS. Pay attention that the NOCOW flags disables the checksum and
prevent scrub to rebuild a corrupted journal.
I now merged patches 1-3/4, but not this one. Setting/unsetting
attributes seems to be generally useful, so the rest stands on its
own. The reason I held back with the last patch is that setting of the
attributes through tmpfiles should be added together with the removal
of the same functionality from journald. But there are some details to
work out.

Setting +C on /var/log/journal/%m has smaller scope than the code in
journal-file.c now. For example it does not cover files opened by
systemd-journal-remote. Having the files no-cow is just as useful for those,
but it's not entirely clear how to support it in the new scheme.

Zbyszek
Post by Goffredo Baroncelli
+# set the journal file as NOCOW; only valid for BTRFS filesystem
+h /var/log/journal/%m - - - - +C
Goffredo Baroncelli
2015-03-20 18:06:28 UTC
Permalink
Hi Zbyszek,
Post by Zbigniew Jędrzejewski-Szmek
Post by Goffredo Baroncelli
Add a new tmpfiles.d snippets to set the NOCOW attributes for the
journal files. This allow better perfomance when the root file system
is BTRFS. Pay attention that the NOCOW flags disables the checksum and
prevent scrub to rebuild a corrupted journal.
I now merged patches 1-3/4, but not this one. Setting/unsetting
attributes seems to be generally useful, so the rest stands on its
own. The reason I held back with the last patch is that setting of the
attributes through tmpfiles should be added together with the removal
of the same functionality from journald.
You are right, the patch #4 and the removal of the current code are coupled;
with the patch #1..#3 included, I will re-issue the #4 with another patch which reverts the code. And the discussion will restart.
Post by Zbigniew Jędrzejewski-Szmek
But there are some details to
work out.
Setting +C on /var/log/journal/%m has smaller scope than the code in
journal-file.c now. For example it does not cover files opened by
systemd-journal-remote.
I am not familiar with s*d-journal-remote; from the man page it seems that the log are stored /by default) in /var/log/journal/remote/ ; if so it is sufficient to add a line like

+h /var/log/journal/remote - - - - +C
Post by Zbigniew Jędrzejewski-Szmek
Having the files no-cow is just as useful for those,
but it's not entirely clear how to support it in the new scheme.
Zbyszek
Post by Goffredo Baroncelli
+# set the journal file as NOCOW; only valid for BTRFS filesystem
+h /var/log/journal/%m - - - - +C
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Zbigniew Jędrzejewski-Szmek
2015-03-22 19:53:13 UTC
Permalink
Post by Goffredo Baroncelli
Hi Zbyszek,
Post by Zbigniew Jędrzejewski-Szmek
Post by Goffredo Baroncelli
Add a new tmpfiles.d snippets to set the NOCOW attributes for the
journal files. This allow better perfomance when the root file system
is BTRFS. Pay attention that the NOCOW flags disables the checksum and
prevent scrub to rebuild a corrupted journal.
I now merged patches 1-3/4, but not this one. Setting/unsetting
attributes seems to be generally useful, so the rest stands on its
own. The reason I held back with the last patch is that setting of the
attributes through tmpfiles should be added together with the removal
of the same functionality from journald.
You are right, the patch #4 and the removal of the current code are coupled;
with the patch #1..#3 included, I will re-issue the #4 with another patch which reverts the code. And the discussion will restart.
Post by Zbigniew Jędrzejewski-Szmek
But there are some details to
work out.
Setting +C on /var/log/journal/%m has smaller scope than the code in
journal-file.c now. For example it does not cover files opened by
systemd-journal-remote.
I am not familiar with s*d-journal-remote; from the man page it seems that the log are stored /by default) in /var/log/journal/remote/ ; if so it is sufficient to add a line like
+h /var/log/journal/remote - - - - +C
That's the problem: current functionality works no matter where you
store the files, but it's hard to provide the same level of
flexibility with the tmpfiles-based solution.

Zbyszek
Goffredo Baroncelli
2015-03-21 07:38:38 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Goffredo Baroncelli
Hi Zbyszek,
[...]
Post by Zbigniew Jędrzejewski-Szmek
Post by Goffredo Baroncelli
Post by Zbigniew Jędrzejewski-Szmek
But there are some details to
work out.
Setting +C on /var/log/journal/%m has smaller scope than the code in
journal-file.c now. For example it does not cover files opened by
systemd-journal-remote.
I am not familiar with s*d-journal-remote; from the man page it seems that the log are stored /by default) in /var/log/journal/remote/ ; if so it is sufficient to add a line like
+h /var/log/journal/remote - - - - +C
That's the problem: current functionality works no matter where you
store the files, but it's hard to provide the same level of
flexibility with the tmpfiles-based solution.
Unfortunately the +C attributes has his downside, which cannot be ignored. All my work was due to the fact that the NOCOW attribute was set unconditionally. I proposed a patches set which adds an option to tune this behavior; Lennart asked to replace it with this "tmpfile.d" solution.

Frankly speaking, I think that it would be doable also to set the -C attributes by hand by the user/sysadmin[1]. My needing is only to avoid this *unconditionally* NOCOW attribute set, for these few cases (BTRFS w/RAID) where it could degrades the btrfs features ( without notifying the user !).
Post by Zbigniew Jędrzejewski-Szmek
Zbyszek
Goffredo

[1] It is needed only one time; after you set as +C the directory all the new files
inherits this attribute.
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Lennart Poettering
2015-04-08 21:48:21 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Goffredo Baroncelli
Hi Zbyszek,
Post by Zbigniew Jędrzejewski-Szmek
Post by Goffredo Baroncelli
Add a new tmpfiles.d snippets to set the NOCOW attributes for the
journal files. This allow better perfomance when the root file system
is BTRFS. Pay attention that the NOCOW flags disables the checksum and
prevent scrub to rebuild a corrupted journal.
I now merged patches 1-3/4, but not this one. Setting/unsetting
attributes seems to be generally useful, so the rest stands on its
own. The reason I held back with the last patch is that setting of the
attributes through tmpfiles should be added together with the removal
of the same functionality from journald.
You are right, the patch #4 and the removal of the current code are coupled;
with the patch #1..#3 included, I will re-issue the #4 with another patch which reverts the code. And the discussion will restart.
Post by Zbigniew Jędrzejewski-Szmek
But there are some details to
work out.
Setting +C on /var/log/journal/%m has smaller scope than the code in
journal-file.c now. For example it does not cover files opened by
systemd-journal-remote.
I am not familiar with s*d-journal-remote; from the man page it seems that the log are stored /by default) in /var/log/journal/remote/ ; if so it is sufficient to add a line like
+h /var/log/journal/remote - - - - +C
That's the problem: current functionality works no matter where you
store the files, but it's hard to provide the same level of
flexibility with the tmpfiles-based solution.
Well, but we never store files outside of /var/log/journal,
/var/log/journal/%m and /var/log/journal/remote/, do we?

I think this is close enough to be useful.

Lennart
--
Lennart Poettering, Red Hat
Zbigniew Jędrzejewski-Szmek
2015-04-11 17:07:57 UTC
Permalink
Post by Lennart Poettering
Post by Zbigniew Jędrzejewski-Szmek
Post by Goffredo Baroncelli
Hi Zbyszek,
Post by Zbigniew Jędrzejewski-Szmek
Post by Goffredo Baroncelli
Add a new tmpfiles.d snippets to set the NOCOW attributes for the
journal files. This allow better perfomance when the root file system
is BTRFS. Pay attention that the NOCOW flags disables the checksum and
prevent scrub to rebuild a corrupted journal.
I now merged patches 1-3/4, but not this one. Setting/unsetting
attributes seems to be generally useful, so the rest stands on its
own. The reason I held back with the last patch is that setting of the
attributes through tmpfiles should be added together with the removal
of the same functionality from journald.
You are right, the patch #4 and the removal of the current code are coupled;
with the patch #1..#3 included, I will re-issue the #4 with another patch which reverts the code. And the discussion will restart.
Post by Zbigniew Jędrzejewski-Szmek
But there are some details to
work out.
Setting +C on /var/log/journal/%m has smaller scope than the code in
journal-file.c now. For example it does not cover files opened by
systemd-journal-remote.
I am not familiar with s*d-journal-remote; from the man page it seems that the log are stored /by default) in /var/log/journal/remote/ ; if so it is sufficient to add a line like
+h /var/log/journal/remote - - - - +C
That's the problem: current functionality works no matter where you
store the files, but it's hard to provide the same level of
flexibility with the tmpfiles-based solution.
Well, but we never store files outside of /var/log/journal,
/var/log/journal/%m and /var/log/journal/remote/, do we?
We can, if instructed to do so. journal-remote can store files wherever.

Original motivation for this patch was to make the NOCOW on journal files
configurable without too much fuss and without making it an explicit option.
Journal files on btrfs without NOCOW are rather slow, so it seems that most
people will want NOCOW on. But with the proposed patch, people would want
to add the tmpfile snippet to set NOCOW for every location they write too,
which is very visible and requires explicit configuration. Doing this in
journal-file directly was simple, synchronous, and worked everywhere, and
we are replacing this with a more complicated and more brittle scheme.

Dunno, if you think things are better this way, I'm fine with that,
since both schemes should get the job done. But in the end, adding a
switch in journald.conf seems more in the systemd spirit of doing the right
thing automatically and also less work for both sides...

Zbyszek
Goffredo Baroncelli
2015-04-12 18:19:29 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Lennart Poettering
Post by Zbigniew Jędrzejewski-Szmek
That's the problem: current functionality works no matter where you
store the files, but it's hard to provide the same level of
flexibility with the tmpfiles-based solution.
Well, but we never store files outside of /var/log/journal,
/var/log/journal/%m and /var/log/journal/remote/, do we?
We can, if instructed to do so. journal-remote can store files wherever.
Original motivation for this patch was to make the NOCOW on journal files
configurable without too much fuss and without making it an explicit option.
Journal files on btrfs without NOCOW are rather slow, so it seems that most
people will want NOCOW on. But with the proposed patch, people would want
to add the tmpfile snippet to set NOCOW for every location they write too,
which is very visible and requires explicit configuration. Doing this in
journal-file directly was simple, synchronous, and worked everywhere, and
we are replacing this with a more complicated and more brittle scheme.
Dunno, if you think things are better this way, I'm fine with that,
since both schemes should get the job done. But in the end, adding a
switch in journald.conf seems more in the systemd spirit of doing the right
thing automatically and also less work for both sides...
What about this solution: let's go the tmpfiles way, but also add some
code to the journal file writer to log at INFO level an actionable
recommendation to turn on the c flag on the directory if we notice
that the newly created file doesn't have it, and it is located on
btrfs?
After the work that I done to the tmpfiles, I have to agree with
Zbyszek. Adding an option to the journal.conf file is the more
reasonable thing to do.
You mean journald.conf I figure? That's not even read by the remoting
tools, so how is that a solution?
In my first attempt, I added a switch to the command line.
- journal code is still aware of the NOCOW attribute (== more code complexity)
- the user had to update/manage a tmpfile.d manually
Well, again: the nocow thing is a work-around around a design issue
with btrfs, and btrfs is working on correcting that, by adding
auto-defrag to deal better with write patterns such as this.
We should not add new explicit config options for things we already
know *now* that they are a stopgap, and will go away eventually.
With the solution I propose (which is tmpfiles snippet + warning if
- default behaviour is fast
- default behaviour is easy to override
- specialist users who use the remoting feature and use the thing on a
non-default directory, are notified about the issue at hand.
- we can easily get rid of it eventually, simply by dropping one
config line and the generation of a warning. There's no option to
deprecate then, and keep compat for.
Even if I agree with you about the points above, I am not fully
convinced about changing the NOCOW attribute via tmpfile snippet:
it seems to me an overkill solution..

But I prefer the snippet solution to the old behavior (set the NOCOW attribute
coded in systemd-journald).
- add a switch in the journald.conf file
- add a check that raise a warning if the NOCOW flag is not-used/used
This does not fix the remoting issue, since journald.conf isn't read
by those tools. Also, it adds a setting we'll eventuall have to get
rid of again.
Sorry, but I am really against a solution like that. I don't want to
litter the config file with options that are hacks and will go away
one day...
Lennart
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Lennart Poettering
2015-04-12 18:24:37 UTC
Permalink
Post by Goffredo Baroncelli
After the work that I done to the tmpfiles, I have to agree with
Zbyszek. Adding an option to the journal.conf file is the more
reasonable thing to do.
You mean journald.conf I figure? That's not even read by the remoting
tools, so how is that a solution?
In my first attempt, I added a switch to the command line.
So we'd even have two new options where we'd know now that they would
go away? And one of them isn't even persistent?

This really sounds wrong. We should be frugal with adding new options,
and this case is extraordinarily weak I think...
Post by Goffredo Baroncelli
Even if I agree with you about the points above, I am not fully
it seems to me an overkill solution..
But I prefer the snippet solution to the old behavior (set the NOCOW attribute
coded in systemd-journald).
If you rebase the patches, and fix the issues I pointed out I am happy
to merge patches that set the c bit on the dirs via tmpfiles, and prints
a warning when creating a new journal file on btrfs where the flag is
off...

Lennart
--
Lennart Poettering, Red Hat
Goffredo Baroncelli
2015-04-12 15:31:22 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Lennart Poettering
Post by Zbigniew Jędrzejewski-Szmek
That's the problem: current functionality works no matter where you
store the files, but it's hard to provide the same level of
flexibility with the tmpfiles-based solution.
Well, but we never store files outside of /var/log/journal,
/var/log/journal/%m and /var/log/journal/remote/, do we?
We can, if instructed to do so. journal-remote can store files wherever.
Original motivation for this patch was to make the NOCOW on journal files
configurable without too much fuss and without making it an explicit option.
Journal files on btrfs without NOCOW are rather slow, so it seems that most
people will want NOCOW on. But with the proposed patch, people would want
to add the tmpfile snippet to set NOCOW for every location they write too,
which is very visible and requires explicit configuration. Doing this in
journal-file directly was simple, synchronous, and worked everywhere, and
we are replacing this with a more complicated and more brittle scheme.
Dunno, if you think things are better this way, I'm fine with that,
since both schemes should get the job done. But in the end, adding a
switch in journald.conf seems more in the systemd spirit of doing the right
thing automatically and also less work for both sides...
What about this solution: let's go the tmpfiles way, but also add some
code to the journal file writer to log at INFO level an actionable
recommendation to turn on the c flag on the directory if we notice
that the newly created file doesn't have it, and it is located on
btrfs?
After the work that I done to the tmpfiles, I have to agree with Zbyszek. Adding an option to the journal.conf file is the more reasonable thing to do.

If we add code that performs only a check in the code of journal, I think that we have the worst solution:
- journal code is still aware of the NOCOW attribute (== more code complexity)
- the user had to update/manage a tmpfile.d manually

Let me to suggest the opposite solution:
- add a switch in the journald.conf file
- add a check that raise a warning if the NOCOW flag is not-used/used

GB
That way, folks who use the tools on non-default directories will at
least be guided to an explanation of the general problem.
Lennart
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Lennart Poettering
2015-04-08 18:37:34 UTC
Permalink
Post by Goffredo Baroncelli
+int change_attr_fd(int fd, unsigned value, unsigned mask) {
+ unsigned old_attr, new_attr;
+
+ assert(fd >= 0);
+
+ if (mask == 0)
+ return 0;
+
+ if (ioctl(fd, FS_IOC_GETFLAGS, &old_attr) < 0)
+ return -errno;
+
+ new_attr = (old_attr & ~mask) |(value & mask);
+
+ if (new_attr == old_attr)
+ return 0;
+
+ if (ioctl(fd, FS_IOC_SETFLAGS, &new_attr) < 0)
+ return -errno;
+
+ return 0;
+}
+
With this added chattr_fd() is kinda redundant, no?

Lennart
--
Lennart Poettering, Red Hat
Lennart Poettering
2015-04-08 18:47:59 UTC
Permalink
Post by Lennart Poettering
Post by Goffredo Baroncelli
+int change_attr_fd(int fd, unsigned value, unsigned mask) {
+ unsigned old_attr, new_attr;
+
+ assert(fd >= 0);
+
+ if (mask == 0)
+ return 0;
+
+ if (ioctl(fd, FS_IOC_GETFLAGS, &old_attr) < 0)
+ return -errno;
+
+ new_attr = (old_attr & ~mask) |(value & mask);
+
+ if (new_attr == old_attr)
+ return 0;
+
+ if (ioctl(fd, FS_IOC_SETFLAGS, &new_attr) < 0)
+ return -errno;
+
+ return 0;
+}
+
With this added chattr_fd() is kinda redundant, no?
I fixed this now.

Lennart
--
Lennart Poettering, Red Hat
Continue reading on narkive:
Loading...