Standardise chmod, chflags and chown option processing

[smh]

Summary

chmod, chflags and chown had sligntly different:

1. -h symlink processing flag validation
2. -v verbose processing

This standardises all three on:

1. Allowing -h with -R but not with -R in combination with -L or -P.
2. Output from -v lists all file processed when used in combination with -f including those which fail.

[smh]

Correct cut and past issue

[smh]

Allow -R -H in combination with -h and update the man pages to reflect.

[eadler]

jilles may want to look at this

[allanjude]

bin/chflags/chflags.c

110

this will cause -L to be silently ignored if -H is specified. should an error be emitted if the two are combined?

bin/chmod/chmod.1

66–68

Symbolic links encountered during tree traversal are not followed

reads better

71

man sentences always start on a new line

bin/chmod/chmod.c

133

same as above

usr.sbin/chown/chown.8

93

new line for new sentence

usr.sbin/chown/chown.c

125–126

this seems to be the existing behaviour for chown. However, not sure if it is correct / intended.

[smh]

man page updates as per allanjude's comments.

[jilles]

If you're changing this, please make the interaction between -h and -R more sensible, as specified by symlink(7) and POSIX (though in the latter case not for chmod and chflags). In that interpretation, it is never necessary to specify -h with -R; instead, -P implies -h, and -H implies -h for those files listed on the command line (not found in the tree walk). This may not be a fully compatible change, but it fixes confusing behaviour that commands like "chown -R u:g tree" leave symlinks alone.

For compatibility, chown will still have to allow -RPh but all other -Rh were not allowed and therefore do not need to be allowed.

bin/chflags/chflags.c

110

The switch above makes it such that -L/-H/-P may be specified multiple times and the last one takes effect, per symlink(7).

bin/chmod/chmod.c

70

This is not the correct type of chmod() and lchmod().

Using fchmodat() (chflagsat(), fchownat()) allows using flags (AT_SYMLINK_NOFOLLOW) instead of function pointers, which may lead to clearer code. See usr.bin/touch for an example.

[smh]

In D2316#14, @jilles wrote:

If you're changing this, please make the interaction between -h and -R more sensible, as specified by symlink(7) and POSIX (though in the latter case not for chmod and chflags)
In that interpretation, it is never necessary to specify -h with -R; instead, -P implies -h, and -H implies -h for those files listed on the command line (not found in the tree walk).

Thanks for looking at this, I'm a little confused by your statement about "-H implies -h" as it seems to contradict symlink(7) specifically:
"The -H flag causes symbolic links specified on the command line to be dereferenced both for the purposes of the action to be performed and the tree walk, and it is as if the user had specified the name of the file to which the symbolic link pointed."

How I interpret that is that -H disables -h behaviour for the files specified on the command line only as they should already be dereferenced.

This would mean the combination of atflags AT_SYMLINK_NOFOLLOW and fts_options FTS_COMFOLLOW would achieve the desired behaviour.

Similarly when -L is specified the use fts_options FTS_LOGICAL would produce the desired behaviour (no symlinks will be affected on the files / directories the link to) as all entries will be dereferenced..

If my interpretation is correct I believe the following should achieve the desired behaviour in all cases:

if (Rflag) {
  // Recursive
  if (Lflag) {
    // Symlinks not effected.
    fts_options = FTS_LOGICAL;
  } else {
    // Symlinks effected
    fts_options = FTS_PHYSICAL;
    atflag = AT_SYMLINK_NOFOLLOW;

    if (Hflag) {
      // Symlinks on the command
      // line not effected.
      fts_options |= FTS_COMFOLLOW;
    }
  }
} else if (hflag) {
  // None Recursive
  // Symlinks effected.
  fts_options = FTS_PHYSICAL;
  atflag = AT_SYMLINK_NOFOLLOW;
} else {
  // None Recursive.
  // Symlinks not effected.
  fts_options = FTS_LOGICAL;
}

bin/chflags/chflags.c

110

Correct the are mutually exclusive:
"The -H, -L and -P options are ignored unless the -R option is specified. In addition, these options override each other and the command's actions are determined by the last one specified."

[jilles]

In D2316#17, @smh wrote:

In D2316#14, @jilles wrote:

If you're changing this, please make the interaction between -h and -R more sensible, as specified by symlink(7) and POSIX (though in the latter case not for chmod and chflags)
In that interpretation, it is never necessary to specify -h with -R; instead, -P implies -h, and -H implies -h for those files listed on the command line (not found in the tree walk).

Thanks for looking at this, I'm a little confused by your statement about "-H implies -h" as it seems to contradict symlink(7) specifically:
"The -H flag causes symbolic links specified on the command line to be dereferenced both for the purposes of the action to be performed and the tree walk, and it is as if the user had specified the name of the file to which the symbolic link pointed."

How I interpret that is that -H disables -h behaviour for the files specified on the command line only as they should already be dereferenced.

Oh right, I should have said -H implies -h for those files found in the tree walk.

This would mean the combination of atflags AT_SYMLINK_NOFOLLOW and fts_options FTS_COMFOLLOW would achieve the desired behaviour.

Similarly when -L is specified the use fts_options FTS_LOGICAL would produce the desired behaviour (no symlinks will be affected on the files / directories the link to) as all entries will be dereferenced..

If my interpretation is correct I believe the following should achieve the desired behaviour in all cases:

if (Rflag) {
  // Recursive
  if (Lflag) {
    // Symlinks not effected.
    fts_options = FTS_LOGICAL;
  } else {
    // Symlinks effected
    fts_options = FTS_PHYSICAL;
    atflag = AT_SYMLINK_NOFOLLOW;

    if (Hflag) {
      // Symlinks on the command
      // line not effected.
      fts_options |= FTS_COMFOLLOW;
    }
  }
} else if (hflag) {
  // None Recursive
  // Symlinks effected.
  fts_options = FTS_PHYSICAL;
  atflag = AT_SYMLINK_NOFOLLOW;
} else {
  // None Recursive.
  // Symlinks not effected.
  fts_options = FTS_LOGICAL;
}

You can't use a common atflag for all files, since for -H, you need to do something different for files specified on the command line (fts_level == FTS_ROOTLEVEL) and files found in the tree walk. It is a bit unfortunate that some part of fts(3) functionality (the decision AT_SYMLINK_NOFOLLOW or 0) needs to be replicated here. I think the following would work, placed in the fts_read loop (untested):

if (Rflag)
  atflag = Lflag || (Hflag && p->fts_level == FTS_ROOTLEVEL) ? 0 : AT_SYMLINK_NOFOLLOW;
else
  atflag = hflag ? AT_SYMLINK_NOFOLLOW : 0;
error = fchmodat(AT_FDCWD, p->fts_accpath, newmode, atflag);

It would be simpler to use AT_SYMLINK_NOFOLLOW iff fts_info == FTS_SL || fts_info == FTS_SLNONE but this is wrong since the utilities must not follow symlinks if they were requested not to, even if someone races replacing a non-symlink with a symlink.

[smh]

In D2316#17, @smh wrote:

If my interpretation is correct I believe the following should achieve the desired behaviour in all cases:

...

Seems FTS_COMFOLLOW is not as straight forward as it sounds, in that it appears to return the fts_info for the followed link but the rest of the details in the FTSENT are from the link itself, which seems very odd given its description and the description of FTS_LOGICAL:

FTS_COMFOLLOW 
This option causes any symbolic link	 specified as a root path to be followed immediately whether or not FTS_LOGICAL is also specified.

FTS_LOGICAL
This option causes the fts routines to return FTSENT structures for the targets of symbolic links instead of the symbolic links themselves. If this option is set, the only symbolic links for which FTSENT structures are returned to the application are those referencing non-existent files. Either FTS_LOGICAL or FTS_PHYSICAL must be provided to the fts_open() function.

I will research some more...

[smh]

Switch recursive processing to match that described in symlink(7) as per jilles@ feedback.

It should be noted that atflags needs to be calculated for each file returned from fts_read as contrary to the current version of FTS(3), only certain fields e.g. fts_info and fts_statp of the returned FTSENT structure reflect the dereferenced symlink entries when doing logical structure traversal as indicated by FTS_COMFOLLOW or FTS_LOGICAL.

This also makes use of fchownat, fchmodat and chflagsat instead of using function pointers to make it easier to follow.

I've ammended the man pages slightly to refect the changes but would welcome feedback specifically in this area.

[smh]

I've created a little test script to help verify the new behaviour, for those interested it can be found here:
chflags, chmod and chown test script

[jilles]

I have some small remarks that apply to all three but I only wrote them in chflags.

chown correctly differs from the others in that it accepts -h with -R (but it now does nothing).

The commit should have Relnotes: yes and some hints should be provided for the release notes, like "chflags, chgrp, chmod and chown now affect symlinks in -RP mode; formerly, symlinks were silently ignored".

bin/chflags/chflags.1

70

typo, uneffected should be unaffected.

88

I think the part "including symlinks by default" is a bit confusing. Given the -P description above already states it's default not to follow symlinks, it may be good to simply remove the four words.

bin/chflags/chflags.c

118

I think the conditional operator is fine here; changing to if does not seem to provide a benefit for the extra lines of code.

170–171

I think this condition is impossible, since FTS_SLNONE only happens when an attempt to follow a symlink fails. Leaving it out completely will let chflagsat() fail (or modify another file's flags in a race) and a proper error message will result.

Alternatively, an error message could be written here. FTS_SLNONE doesn't currently come with a valid fts_error but an errno message is not strictly necessary.

[smh]

In D2316#42901, @jilles wrote:

chown correctly differs from the others in that it accepts -h with -R (but it now does nothing).

-R implies -h now so I don't believe it needs to do anything different from its standard processing.

bin/chflags/chflags.c

118

Personally I prefer the long hand when its in combination with other conditionals like this, but if others agree I'm happy to change.

170–171

That would indeed be a nice additional instead of the original behaviour.

[smh]

Updated as per comments from jilles@ also added an UPDATING entry for visibility.

[jilles]

Looks good to me.

Note that I have not tested this.