Hi Miod,
Miod Vallat wrote on Wed, Sep 24, 2014 at 04:27:26PM +0200:
> There is only one goto in chmod.c. If you consider it unnecessary, I'd
> advise you to read the code again, and pay attention to the comment
> explaining that particular chunk.
Heh. "Read the code" is almost always good advice (except when
abused to dismiss reports about defective manuals, maybe :).
So let's turn this thread into something useful and tech@-worthy
and fix an actual bug. The --optind happens in too few cases.
It does not only need to happen when the mode argument stands
alone, but also when it merely stands last in a group, in which
case getopt() has also moved past it.
Right now, in that case, the invalid syntax is silently ignored,
and if any valid mode arguments follow, they take effect.
For example, right now:
$ touch tf
$ chmod 755 tf && chmod -Pr -w tf ; ls -l tf # pun intended
-r-xr-xr-x 1 ischwarze wsrc 0 Sep 24 18:14 tf
Oops, "-Pr" is bad and the -r must not be silently ignored.
With my patch:
$ chmod 755 tf && ./obj/chmod -Pr -w tf ; ls -l tf
chmod: invalid file mode: -Pr
-rwxr-xr-x 1 ischwarze wsrc 0 Sep 24 18:14 tf
OK?
Ingo
P.S.
Regarding the code quality of chmod.c, Miod and Henri have brought
up various good points, so i'll refrain from more comments on style
for now.
Index: chmod.c
===================================================================
RCS file: /cvs/src/bin/chmod/chmod.c,v
retrieving revision 1.30
diff -u -p -r1.30 chmod.c
--- chmod.c 21 May 2014 06:23:01 -0000 1.30
+++ chmod.c 24 Sep 2014 16:40:19 -0000
@@ -107,19 +107,23 @@ main(int argc, char *argv[])
hflag = 1;
break;
/*
- * XXX
- * "-[rwx]" are valid mode commands. If they are the entire
- * argument, getopt has moved past them, so decrement optind.
- * Regardless, we're done argument processing.
+ * If this is a symbolic mode argument rather than
+ * an option, we are done with option processing.
*/
case 'g': case 'o': case 'r': case 's':
case 't': case 'u': case 'w': case 'X': case 'x':
if (!ischmod)
usage();
- if (argv[optind - 1][0] == '-' &&
- argv[optind - 1][1] == ch &&
- argv[optind - 1][2] == '\0')
- --optind;
+ /*
+ * If getopt() moved past the argument, back up.
+ * If the argument contains option letters before
+ * mode letters, setmode() will catch them.
+ */
+ if (optind > 1) {
+ cp = argv[optind - 1];
+ if (cp[strlen(cp) - 1] == ch)
+ --optind;
+ }
goto done;
default:
usage();
