Hello, thanks for review and objections. Paul Eggert wrote: > Ondřej Vašík <[EMAIL PROTECTED]> writes: > > > - bool changed = (chmod_succeeded > > - && mode_changed (file, old_mode, new_mode)); > > + bool mode_change = mode_changed (file, old_mode, new_mode); > > + bool changed = (chmod_succeeded && mode change);
Sorry, that change was obviously redundant...
> > +
> > + if (chmod_succeeded && ((old_mode ^ new_mode) & CHMOD_MODE_BITS))
> > + {
> > +
> > + /* Changed to another mode than requested */
>
> This doesn't look right to me. First, surely there's no need to invoke
> mode_changed if chmod_succeeded is false. Second, ((old_mode ^
> new_mode) & CHMOD_MODE_BITS) doesn't tell us whether we changed to
> another mode than requested; mode_change tells us that.
About first you are right, that change was accidently added for other
approach and I forgot to remove it.
About second - that's not everytime true - as shown in the example -
SUID, SGID and sticky bits could be ignored in mode changed. That's
documented in info changes. Anyway user is then mistakenly informed that
mode was changed e.g. to 2755, but the real change was to 0755 - and
that's imho wrong.
> > + struct stat new_stats;
> > + char perms_requested[12];
> > + char perms_actual[12];
> > +
> > + if (stat (file, &new_stats) != 0)
>
> Third, this means we've invoked 'stat' twice on the file afterwards,
> once here, and once in mode_changed. We should invoke 'stat' only in
> mode_changed.
You are right, that stat() call easy to reduce. So better solution for
that issue would be to check everything in mode_changed() function.
Proposed amended patch is attached, should solve all your objections
about the previous patch.
Greetings,
Ondřej Vašík
From ea697d3aa266f2d281b098b8c0b98157ed10e9b8 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= <[EMAIL PROTECTED]>
Date: Fri, 24 Oct 2008 17:24:09 +0200
Subject: [PATCH] chmod: inform in verbose if used mode for chmod was different than requested
* chmod (mode changed): Display a message in verbose when SUID, SGID or
sticky bit change was requested but not performed. Suggested by Aaron Toponce
* NEWS: Mention that change
---
NEWS | 3 +++
src/chmod.c | 28 ++++++++++++++++++++++------
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/NEWS b/NEWS
index 3fc0349..86f415b 100644
--- a/NEWS
+++ b/NEWS
@@ -23,6 +23,9 @@ GNU coreutils NEWS -*- outline -*-
Rm was improved directly, while the others inherit the improvement
from the newer version of fts in gnulib.
+ chmod now displays a message when SUID, SGID or sticky bit change was
+ requested, but not performed.
+
comm now verifies that the inputs are in sorted order. This check can
be turned off with the --nocheck-order option.
diff --git a/src/chmod.c b/src/chmod.c
index 80fc363..3cdbf61 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -110,9 +110,9 @@ static struct option const long_options[] =
The old mode was OLD_MODE, but it was changed to NEW_MODE. */
static bool
-mode_changed (char const *file, mode_t old_mode, mode_t new_mode)
+mode_changed (char const *file, mode_t old_mode, mode_t *new_mode)
{
- if (new_mode & (S_ISUID | S_ISGID | S_ISVTX))
+ if (*new_mode & (S_ISUID | S_ISGID | S_ISVTX))
{
/* The new mode contains unusual bits that the call to chmod may
have silently cleared. Check whether they actually changed. */
@@ -125,11 +125,27 @@ mode_changed (char const *file, mode_t old_mode, mode_t new_mode)
error (0, errno, _("getting new attributes of %s"), quote (file));
return false;
}
-
- new_mode = new_stats.st_mode;
+ if (((new_stats.st_mode ^ *new_mode) & CHMOD_MODE_BITS) &&
+ (verbosity == V_high) && ((old_mode ^ *new_mode) & CHMOD_MODE_BITS))
+ {
+ /* Do we ignore SUID/SGID/sticky bit? Inform user in verbose! */
+ strmode (*new_mode, perms_requested);
+ perms_requested[10] = '\0';
+ strmode (new_stats.st_mode, perms_actual);
+ perms_actual[10] = '\0';
+ printf(
+ _("can't change mode of %s to %04lo (%s), using %04lo (%s)\n"),
+ quote (file),
+ (unsigned long int) (*new_mode & CHMOD_MODE_BITS),
+ &perms_requested[1],
+ (unsigned long int) (new_stats.st_mode & CHMOD_MODE_BITS),
+ &perms_actual[1]);
+ }
+
+ *new_mode = new_stats.st_mode;
}
- return ((old_mode ^ new_mode) & CHMOD_MODE_BITS) != 0;
+ return ((old_mode ^ *new_mode) & CHMOD_MODE_BITS) != 0;
}
/* Tell the user how/if the MODE of FILE has been changed.
@@ -260,7 +276,7 @@ process_file (FTS *fts, FTSENT *ent)
if (verbosity != V_off)
{
bool changed = (chmod_succeeded
- && mode_changed (file, old_mode, new_mode));
+ && mode_changed (file, old_mode, &new_mode));
if (changed || verbosity == V_high)
{
--
1.5.6.1.156.ge903b
signature.asc
Description: Toto je digitálně podepsaná část zprávy
_______________________________________________ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
