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 <[email protected]>
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);
}
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
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
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
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" <[email protected]>
---
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
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" <[email protected]>
Signed-off-by: Eric Sandeen <[email protected]>
---
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
We can't set the flags on symbolic links, so check for them using
lstat().
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
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