[PATCH e2fsprogs] return status from chattr

[PATCH e2fsprogs] return status from chattr

[Eric Sandeen]

This is for RH bug #180596, Chattr command doesn't provide expected 
exit code in case of failure.

(trying to clear out an e2fsprogs bug backlog, can you tell?)  :)

This is a little funky as a result of the man page saying that
links encountered on recursive traversal are (silently?) ignored.

I changed this a bit so that if it's explicitly listed on the
commandline, the link itself gets chattr'd.  I'm not quite sure 
what is intended here; that the links are not *followed* or
that they are not chattr'd?  Seems a little odd to me.

I tried to follow the way other recursive commands work, for example
chmod -R, and carry on in the face of any errors.  If any error
was encountered, exit with an error.  If no errors, exit 0.

Also, if both flags and -v (version) are specified, and the flag 
set encounters an error, the version set is not attempted.  Is this 
ok or should both commands be tried?

Finally, I'm curious, the utility ignores anything that's not a link,
regular file, or dir, but the kernel code doesn't have these checks.
Should it?

Comments?

Thanks,
-Eric

Signed-off-by: Eric Sandeen <sandeen@redhat.com>


Index: e2fsprogs-1.40.2/misc/chattr.c
===================================================================
--- e2fsprogs-1.40.2.orig/misc/chattr.c
+++ e2fsprogs-1.40.2/misc/chattr.c
@@ -182,7 +182,7 @@ static int decode_arg (int * i, int argc
 
 static int chattr_dir_proc (const char *, struct dirent *, void *);
 
-static void change_attributes (const char * name)
+static int change_attributes (const char * name, int cmdline)
 {
 	unsigned long flags;
 	STRUCT_STAT	st;
@@ -190,19 +190,20 @@ static void change_attributes (const cha
 	if (LSTAT (name, &st) == -1) {
 		com_err (program_name, errno, _("while trying to stat %s"), 
 			 name);
-		return;
+		return -1;
 	}
-	if (S_ISLNK(st.st_mode) && recursive)
-		return;
 
-	/* Don't try to open device files, fifos etc.  We probably
-           ought to display an error if the file was explicitly given
-           on the command line (whether or not recursive was
-           requested).  */
-	if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode) &&
-	    !S_ISDIR(st.st_mode))
-		return;
+	/* Just silently ignore links found by recursion;
+	   not an error according to the manpage */
+	if (S_ISLNK(st.st_mode) && !cmdline)
+		return 0;
 
+	/* Don't try to open device files, fifos etc. */
+	if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode) &&
+	    !S_ISDIR(st.st_mode)) {
+		com_err (program_name, EINVAL, _("for file %s"), name);
+		return -1;
+	}
 	if (set) {
 		if (verbose) {
 			printf (_("Flags of %s set as "), name);
@@ -212,10 +213,11 @@ static void change_attributes (const cha
 		if (fsetflags (name, sf) == -1)
 			perror (name);
 	} else {
-		if (fgetflags (name, &flags) == -1)
+		if (fgetflags (name, &flags) == -1) {
 			com_err (program_name, errno,
 			         _("while reading flags on %s"), name);
-		else {
+			return -1;
+		} else {
 			if (rem)
 				flags &= ~rf;
 			if (add)
@@ -227,25 +229,32 @@ static void change_attributes (const cha
 			}
 			if (!S_ISDIR(st.st_mode))
 				flags &= ~EXT2_DIRSYNC_FL;
-			if (fsetflags (name, flags) == -1)
+			if (fsetflags (name, flags) == -1) {
 				com_err (program_name, errno,
 				         _("while setting flags on %s"), name);
+				return -1;
+			}
+
 		}
 	}
 	if (set_version) {
 		if (verbose)
 			printf (_("Version of %s set as %lu\n"), name, version);
-		if (fsetversion (name, version) == -1)
+		if (fsetversion (name, version) == -1) {
 			com_err (program_name, errno,
 			         _("while setting version on %s"), name);
+			return -1;
+		}
 	}
 	if (S_ISDIR(st.st_mode) && recursive)
-		iterate_on_dir (name, chattr_dir_proc, NULL);
+		return iterate_on_dir (name, chattr_dir_proc, NULL);
 }
 
 static int chattr_dir_proc (const char * dir_name, struct dirent * de,
 			    void * private EXT2FS_ATTR((unused)))
 {
+	int err;
+
 	if (strcmp (de->d_name, ".") && strcmp (de->d_name, "..")) {
 	        char *path;
 
@@ -253,11 +262,13 @@ static int chattr_dir_proc (const char *
 		if (!path) {
 			fprintf(stderr, _("Couldn't allocate path variable "
 					  "in chattr_dir_proc"));
-			exit(1);
+			return -1;
 		}
 		sprintf (path, "%s/%s", dir_name, de->d_name);
-		change_attributes (path);
+		err = change_attributes (path, 0);
 		free(path);
+		if (err)
+			return -1;
 	}
 	return 0;
 }
@@ -266,6 +277,7 @@ int main (int argc, char ** argv)
 {
 	int i, j;
 	int end_arg = 0;
+	int err, retval = 0;
 
 #ifdef ENABLE_NLS
 	setlocale(LC_MESSAGES, "");
@@ -303,7 +315,10 @@ int main (int argc, char ** argv)
 	if (verbose)
 		fprintf (stderr, "chattr %s (%s)\n",
 			 E2FSPROGS_VERSION, E2FSPROGS_DATE);
-	for (j = i; j < argc; j++)
-		change_attributes (argv[j]);
-	exit(0);
+	for (j = i; j < argc; j++) {
+		err = change_attributes (argv[j], 1);
+		if (err)
+			retval = 1;
+	}
+	exit(retval);
 }

Re: [PATCH e2fsprogs] return status from chattr

[Eric Sandeen]

Eric Sandeen wrote:
> This is for RH bug #180596, Chattr command doesn't provide expected 
> exit code in case of failure.
> 
> (trying to clear out an e2fsprogs bug backlog, can you tell?)  :)

Any comments on this one?  Can we commit it if it looks ok?

btw:

Addresses-Red-Hat-Bugzilla: #180596

Thanks,
-Eric

Re: [PATCH e2fsprogs] return status from chattr

[Theodore Tso]

On Thu, Sep 20, 2007 at 12:57:14PM -0500, Eric Sandeen wrote:
> I changed this a bit so that if it's explicitly listed on the
> commandline, the link itself gets chattr'd.  I'm not quite sure 
> what is intended here; that the links are not *followed* or
> that they are not chattr'd?  Seems a little odd to me.

Both; you can't chattr a symlink.  You can try, but in fact what
happens is that you end up chattr'ing the destination of the link,
which is probably not what you want.

> I tried to follow the way other recursive commands work, for example
> chmod -R, and carry on in the face of any errors.  If any error
> was encountered, exit with an error.  If no errors, exit 0.

Yep, that makes sense.

> Also, if both flags and -v (version) are specified, and the flag 
> set encounters an error, the version set is not attempted.  Is this 
> ok or should both commands be tried?

I think an atomic failure makes the most amount of sense.

> Finally, I'm curious, the utility ignores anything that's not a link,
> regular file, or dir, but the kernel code doesn't have these checks.
> Should it?

For better or for worse, the ext2/3/4 flags are set via an ioctl.
This means it doesn't work for anything other than a regular file or a
directory.  For a symlink, it will fail, or follow the symlink.
There's a bug in libe2p which I'll fix which causes it to not return
EOPNOTSUPP for symlinks because it's using stat() when it should use
lstat().

So we should be ignoring symbolic links, as well, since we can't
change them.

						- Ted

Re: [PATCH e2fsprogs] return status from chattr

[Theodore Tso]

On Fri, Oct 19, 2007 at 01:55:07PM -0500, Eric Sandeen wrote:
> Eric Sandeen wrote:
> > This is for RH bug #180596, Chattr command doesn't provide expected 
> > exit code in case of failure.
> > 
> > (trying to clear out an e2fsprogs bug backlog, can you tell?)  :)
> 
> Any comments on this one?  Can we commit it if it looks ok?

It turns out you didn't quite completely fix the problem, since the
iterate_on_directory() command wasn't reflecting errors up.

Also, while I was at it I cleaned up the code so that it more it
attempts to set the flags on special files, instead of just ignoring
them.  On Linux this will result in an error message, but at least
this way the user knows that those files didn't get set, and you can
suppress them using the -f flag, just like chmod/chown.

						- Ted

[PATCH] libe2p: Use lstat() instead of stat() in fsetflags() and fgetflags()

[Theodore Ts'o]

We can't set the flags on symbolic links, so check for them using
lstat().

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/e2p/fgetflags.c |    2 +-
 lib/e2p/fsetflags.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/e2p/fgetflags.c b/lib/e2p/fgetflags.c
index 0aed6c8..372304f 100644
--- a/lib/e2p/fgetflags.c
+++ b/lib/e2p/fgetflags.c
@@ -65,7 +65,7 @@ int fgetflags (const char * name, unsigned long * flags)
 #if HAVE_EXT2_IOCTLS
 	int fd, r, f, save_errno = 0;
 
-	if (!stat(name, &buf) &&
+	if (!lstat(name, &buf) &&
 	    !S_ISREG(buf.st_mode) && !S_ISDIR(buf.st_mode)) {
 		goto notsupp;
 	}
diff --git a/lib/e2p/fsetflags.c b/lib/e2p/fsetflags.c
index 2f1277f..3a05324 100644
--- a/lib/e2p/fsetflags.c
+++ b/lib/e2p/fsetflags.c
@@ -71,7 +71,7 @@ int fsetflags (const char * name, unsigned long flags)
 #if HAVE_EXT2_IOCTLS
 	int fd, r, f, save_errno = 0;
 
-	if (!stat(name, &buf) &&
+	if (!lstat(name, &buf) &&
 	    !S_ISREG(buf.st_mode) && !S_ISDIR(buf.st_mode)) {
 		goto notsupp;
 	}
-- 
1.5.3.4.1232.g9991d-dirty

[PATCH] libe2p: Change iterate_on_dir so that it counts non-zero returns

[Theodore Ts'o]

To allow error messages to be reflected up, if the callback function
returns a non-zero value, bump a counter and return the number of
times the callback function signals an error by returning a non-zero
status code.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/e2p/iod.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/e2p/iod.c b/lib/e2p/iod.c
index 808d3a3..84c9709 100644
--- a/lib/e2p/iod.c
+++ b/lib/e2p/iod.c
@@ -27,7 +27,7 @@ int iterate_on_dir (const char * dir_name,
 {
 	DIR * dir;
 	struct dirent *de, *dep;
-	int	max_len = -1, len;
+	int	max_len = -1, len, ret = 0;
 
 #if HAVE_PATHCONF && defined(_PC_NAME_MAX) 
 	max_len = pathconf(dir_name, _PC_NAME_MAX);
@@ -64,9 +64,10 @@ int iterate_on_dir (const char * dir_name,
 			len = max_len;
 #endif
 		memcpy(de, dep, len);
-		(*func) (dir_name, de, private);
+		if ((*func)(dir_name, de, private))
+			ret++;
 	}
 	free(de);
 	closedir(dir);
-	return 0;
+	return ret;
 }
-- 
1.5.3.4.1232.g9991d-dirty

[PATCH] chattr: provide an exit code in case of failure and add -f flag

[Theodore Ts'o]

Fix chattr so that if there are errors, it will report it via a
non-zero exit code.  It will now explicitly give errors when
attempting to set files that are not files or directories (which are
currently not supported under Linux).  The -f flag will suppress error
messages from being printed, although the exit status will still be
non-zero.

Addresses-Red-Hat-Bugzilla: #180596

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 misc/chattr.1.in |    7 ++--
 misc/chattr.c    |   83 +++++++++++++++++++++++++++++++----------------------
 2 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/misc/chattr.1.in b/misc/chattr.1.in
index 2b48fb0..2334675 100644
--- a/misc/chattr.1.in
+++ b/misc/chattr.1.in
@@ -5,7 +5,7 @@ chattr \- change file attributes on a Linux second extended file system
 .SH SYNOPSIS
 .B chattr
 [
-.B \-RV
+.B \-RVf
 ]
 [
 .B \-v
@@ -34,12 +34,13 @@ synchronous updates (S), and top of directory hierarchy (T).
 .TP
 .B \-R
 Recursively change attributes of directories and their contents.
-Symbolic links encountered during recursive directory traversals are
-ignored.
 .TP
 .B \-V
 Be verbose with chattr's output and print the program version.
 .TP
+.B \-f
+Suppress most error messages.
+.TP
 .BI \-v " version"
 Set the file's version/generation number.
 .SH ATTRIBUTES
diff --git a/misc/chattr.c b/misc/chattr.c
index c6d8d9f..efaa559 100644
--- a/misc/chattr.c
+++ b/misc/chattr.c
@@ -65,6 +65,7 @@ static unsigned long version;
 
 static int recursive;
 static int verbose;
+static int silent;
 
 static unsigned long af;
 static unsigned long rf;
@@ -80,8 +81,8 @@ static unsigned long sf;
 
 static void usage(void)
 {
-	fprintf(stderr, 
-		_("Usage: %s [-RV] [-+=AacDdijsSu] [-v version] files...\n"),
+	fprintf(stderr,
+		_("Usage: %s [-RVf] [-+=AacDdijsSu] [-v version] files...\n"),
 		program_name);
 	exit(1);
 }
@@ -137,6 +138,10 @@ static int decode_arg (int * i, int argc, char ** argv)
 				verbose = 1;
 				continue;
 			}
+			if (*p == 'f') {
+				silent = 1;
+				continue;
+			}
 			if (*p == 'v') {
 				(*i)++;
 				if (*i >= argc)
@@ -144,7 +149,7 @@ static int decode_arg (int * i, int argc, char ** argv)
 				version = strtol (argv[*i], &tmp, 0);
 				if (*tmp) {
 					com_err (program_name, 0,
-						 _("bad version - %s\n"), 
+						 _("bad version - %s\n"),
 						 argv[*i]);
 					usage ();
 				}
@@ -182,26 +187,17 @@ static int decode_arg (int * i, int argc, char ** argv)
 
 static int chattr_dir_proc (const char *, struct dirent *, void *);
 
-static void change_attributes (const char * name)
+static int change_attributes (const char * name, int cmdline)
 {
 	unsigned long flags;
 	STRUCT_STAT	st;
 
 	if (LSTAT (name, &st) == -1) {
-		com_err (program_name, errno, _("while trying to stat %s"), 
-			 name);
-		return;
+		if (!silent)
+			com_err (program_name, errno,
+				 _("while trying to stat %s"), name);
+		return -1;
 	}
-	if (S_ISLNK(st.st_mode) && recursive)
-		return;
-
-	/* Don't try to open device files, fifos etc.  We probably
-           ought to display an error if the file was explicitly given
-           on the command line (whether or not recursive was
-           requested).  */
-	if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode) &&
-	    !S_ISDIR(st.st_mode))
-		return;
 
 	if (set) {
 		if (verbose) {
@@ -212,10 +208,12 @@ static void change_attributes (const char * name)
 		if (fsetflags (name, sf) == -1)
 			perror (name);
 	} else {
-		if (fgetflags (name, &flags) == -1)
-			com_err (program_name, errno,
-			         _("while reading flags on %s"), name);
-		else {
+		if (fgetflags (name, &flags) == -1) {
+			if (!silent)
+				com_err (program_name, errno,
+					 _("while reading flags on %s"), name);
+			return -1;
+		} else {
 			if (rem)
 				flags &= ~rf;
 			if (add)
@@ -227,25 +225,36 @@ static void change_attributes (const char * name)
 			}
 			if (!S_ISDIR(st.st_mode))
 				flags &= ~EXT2_DIRSYNC_FL;
-			if (fsetflags (name, flags) == -1)
-				com_err (program_name, errno,
-				         _("while setting flags on %s"), name);
+			if (fsetflags (name, flags) == -1) {
+				if (!silent)
+					com_err(program_name, errno,
+						_("while setting flags on %s"),
+						name);
+				return -1;
+			}
 		}
 	}
 	if (set_version) {
 		if (verbose)
 			printf (_("Version of %s set as %lu\n"), name, version);
-		if (fsetversion (name, version) == -1)
-			com_err (program_name, errno,
-			         _("while setting version on %s"), name);
+		if (fsetversion (name, version) == -1) {
+			if (!silent)
+				com_err (program_name, errno,
+					 _("while setting version on %s"),
+					 name);
+			return -1;
+		}
 	}
 	if (S_ISDIR(st.st_mode) && recursive)
-		iterate_on_dir (name, chattr_dir_proc, NULL);
+		return iterate_on_dir (name, chattr_dir_proc, NULL);
+	return 0;
 }
 
 static int chattr_dir_proc (const char * dir_name, struct dirent * de,
 			    void * private EXT2FS_ATTR((unused)))
 {
+	int ret = 0;
+
 	if (strcmp (de->d_name, ".") && strcmp (de->d_name, "..")) {
 	        char *path;
 
@@ -253,19 +262,20 @@ static int chattr_dir_proc (const char * dir_name, struct dirent * de,
 		if (!path) {
 			fprintf(stderr, _("Couldn't allocate path variable "
 					  "in chattr_dir_proc"));
-			exit(1);
+			return -1;
 		}
-		sprintf (path, "%s/%s", dir_name, de->d_name);
-		change_attributes (path);
+		sprintf(path, "%s/%s", dir_name, de->d_name);
+		ret = change_attributes(path, 0);
 		free(path);
 	}
-	return 0;
+	return ret;
 }
 
 int main (int argc, char ** argv)
 {
 	int i, j;
 	int end_arg = 0;
+	int err, retval = 0;
 
 #ifdef ENABLE_NLS
 	setlocale(LC_MESSAGES, "");
@@ -303,7 +313,10 @@ int main (int argc, char ** argv)
 	if (verbose)
 		fprintf (stderr, "chattr %s (%s)\n",
 			 E2FSPROGS_VERSION, E2FSPROGS_DATE);
-	for (j = i; j < argc; j++)
-		change_attributes (argv[j]);
-	exit(0);
+	for (j = i; j < argc; j++) {
+		err = change_attributes (argv[j], 1);
+		if (err)
+			retval = 1;
+	}
+	exit(retval);
 }
-- 
1.5.3.4.1232.g9991d-dirty