Add Recursive Functionality to setfacl(1)

[allanjude]

Summary

This patch has been in bugzilla since 2011, but has been updated for -current
PR: 155163

[emaste]

bin/setfacl/setfacl.c

43

sort order?

70–78

would be nice to rationalize the existing boolean/flags

[allanjude]

bin/setfacl/setfacl.c

70–78

I am unclear what you mean here. h_flag was moved up to the top scope so it is accessible in the walk_path() function

[allanjude]

Alphabetize the includes

[rwatson]

Adding "-R" support is a good idea.

Would be good to have @trasz review NFSv4-related parts of this patch.

It would be good to also add "-R" support to getfacl(1), getextattr(1), setextattr(1), etc.

bin/setfacl/setfacl.1

29

Should update the date stamp here?

120

Probably slightly better English to go with "Recurse into the..."? Hard to say with computer science, though.

Should have a hard line break before the sentence beginning "NFSv4 inheritance..".

Are the flag removed from the file, or are they simply removed from the requested change?

bin/setfacl/setfacl.c

70–78

Also: elsewhere in the file, "uint" appears to be preferred to "unsigned int:" for carried_error,

81

Possibly more line wrappery required here (hard to say in Fabricator).

110

style(9) wouldn't mind if you had fewer vertical blank lines in the body of this function. But likes the one after local-variable declarations.

131

style(9) prefers that this read "return (acl_new);"

135

Perhaps should line wrap at 78 characters?

187

Comment should be a complete sentence including punctuation.

292

style(9) expects tab plus four-character indent expected for continued line here (and elsewhere).

[allanjude]

Address style feedback from rwatson

[jilles]

I like the idea of adding -R support, but I think the way symlinks are handled is dangerous: with the proposed code, setfacl -R will follow symlinks. This should be changed to match symlink(7), adding -L, -H and -P options.

Using ntfw is acceptable, but most existing utilities use fts.

[emaste]

nftw is POSIX so perhaps slightly preferable. I'm indifferent.

bin/setfacl/setfacl.c

70–78

I mean giving the old and new flags the same staticness.

419

These changes ought to go in separately, and if these are becoming sentences they should also start with a capital letter.

[emaste]

bin/setfacl/setfacl.c

70–78

Oh, need_mask and n_flag are used by multiple .c files. have_mask and have_stdin could be static, but seems sensible to leave them alone. So I think this part is fine as is.

[allanjude]

bin/setfacl/setfacl.c

70–78

Turns out these are like this on purpose. The non-static ones are used in other files (remove.c, merge.c, mask.c) and the static ones are only used in this file.

[allanjude]

Cleanup comments, only fix new comments

[wblock]

bin/setfacl/setfacl.1

120

I suspect s/that/which/ is more correct. Although this whole sentence is kind of a tautology: "Directory-only flags are for directories only."

[jilles]

In D9096#188667, @emaste wrote:

nftw is POSIX so perhaps slightly preferable. I'm indifferent.

I looked at it somewhat deeper and I think nftw is not good enough. In the man page for nftw, no equivalent for the FTS_COMFOLLOW flag can be found (which is necessary to distinguish -RP and -RH operation), and the implementation in lib/libc/gen/nftw.c always sets FTS_COMFOLLOW, so that the proper behaviour for -R and -RP cannot be implemented (which is not to follow any symlinks).

Although I agree in general that POSIX functions should be used when they have sufficient functionality, I don't think nftw has sufficient functionality. Any implementation of -R using nftw will be messy and vulnerable to race conditions.

Perhaps our nftw implementation is wrong to set FTS_COMFOLLOW, but in that case there would be a problem implementing -RH. There would not be a race condition but the code will be messier than needed.

A simple example of implementing -R using fts can be found in bin/chmod/chmod.c.

[emaste]

fts-based implementation in D14934

[emaste]

D14934 version committed in rS332396: setfacl: add recursive functionality

[emaste]

I think this can be abandoned now?