This patch has been in bugzilla since 2011, but has been updated for -current
PR: 155163
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 6649 Build 6867: arc lint + arc unit
Event Timeline
| 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 | |
Comment Actions
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). | |
| 109 | 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. | |
| 130 | style(9) prefers that this read "return (acl_new);" | |
| 134 | Perhaps should line wrap at 78 characters? | |
| 186 | Comment should be a complete sentence including punctuation. | |
| 291 | style(9) expects tab plus four-character indent expected for continued line here (and elsewhere). | |
Comment Actions
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.
| 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. | |
| 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. | |
| 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." | |
Comment Actions
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.
add these in alpha order probably