Page MenuHomeFreeBSD
Differential D28255

Add some more non-portable acl_* functions.
ClosedPublic
Actions

Authored by arrowd on Jan 20 2021, 3:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 30, 1:30 PM
Unknown Object (File)
Fri, Mar 29, 3:19 PM
Unknown Object (File)
Thu, Mar 28, 12:43 PM
Unknown Object (File)
Jan 22 2024, 10:57 PM
Unknown Object (File)
Jan 13 2024, 12:38 AM
Unknown Object (File)
Jan 7 2024, 2:08 AM
Unknown Object (File)
Jan 7 2024, 2:08 AM
Unknown Object (File)
Jan 7 2024, 2:08 AM
View All 77 Files
Subscribers
debdrup
gbe
imp
kib

Details

Reviewers
trasz
kib
Group Reviewers
Contributor Reviews (src)
docs
HardenedBSD
manpages
Commits
rGc468923b2210: libc/posix1e: Add acl_extended_file_np() function.
rGf8b88be850a5: tests/sys/acl: Add ATF C test for newly added acl_* functions.
rGd81d5b2f2c9c: libc/posix1e: Add acl_equiv_mode_np() function.
rGfcef0684f108: Fix build of bin/getfacl after libc changes.
rG937f807a3675: libc/posix1e: Add acl_cmp_np() function.
rG19f7f91133af: libc/posix1e: Add acl_from_mode_np() function.
Summary

This review adds acl_from_mode_np, acl_cmp_np and acl_equiv_mode_np functions.
These functions without _np prefix are present in Linux libacl and some software
(KDE, specifically) uses them along with standard POSIX ACL functions.

It is impossible to implement some of them outside of libc, and since we already
have a bit of *_np functions, I decided to implement these.

Test Plan

I did no testing for these functions. I guess, getting this in requires adding
some regressions tests? If yes, I need a guide on how to write and execute tests,
and then I'm happy to do the work.

Special attention should be payed to man pages - I both non-native English
speaker and non-proficient with mandoc.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 36372
Build 33261: arc lint + arc unit

Event Timeline

arrowd created this revision.Jan 20 2021, 3:55 PM
Herald added a reviewer: manpages. · View Herald TranscriptJan 20 2021, 3:55 PM
Herald added a subscriber: imp. · View Herald Transcript
arrowd requested review of this revision.Jan 20 2021, 3:55 PM
Harbormaster completed remote builds in B36369: Diff 82637.Jan 20 2021, 3:55 PM
debdrup added a subscriber: debdrup.Jan 20 2021, 4:27 PM

Interacting with kyau, the test suite, is described here on the wiki.

lib/libc/posix1e/acl_cmp_np.3
54

Another instance of referring to spec making it seem to differ from implementation, as described below.

56

Same as above.

lib/libc/posix1e/acl_equiv_mode_np.3
27

I don't believe &FreeBSD& is needed or encouraged anymore, as it's not used by git - but someone else should probably confirm this.

45

You missed a word here.

47

A bit of wordsmithing to help ledgibility

54–55

Is it supposed to be 'mode values or 'a mode value'?

57

Missed a letter here.

68

Same as above.

lib/libc/posix1e/acl_from_mode_np.3
52

I think it's better to stick with terminology that relates to the VM subsystem here? The dash is optional, but I think it looks better with it.

58

'shall return' makes it sounds like a spec is being quoted, and that the implementation doesn't follow this - I assume that's not the intention?

63

Same as above.

65

Same as above.

69

Same as above.

arrowd updated this revision to Diff 82647.Jan 20 2021, 6:43 PM
arrowd marked 12 inline comments as done.
  • Address comments on man pages.
Harbormaster completed remote builds in B36372: Diff 82647.Jan 20 2021, 6:43 PM
arrowd added a comment.Jan 20 2021, 6:43 PM

Interacting with kyau, the test suite, is described here on the wiki.

Thanks for the link, I'll look into that.

lib/libc/posix1e/acl_equiv_mode_np.3
54–55

The latter, thanks.

lib/libc/posix1e/acl_from_mode_np.3
58

That's right - there is no spec as these functions are non-standard.

debdrup added a comment.Jan 21 2021, 2:14 AM

From a manual page point of view, this looks good to me.

I can't speak to the actual code and whether it matches the documentation, though.

arrowd updated this revision to Diff 83159.Jan 30 2021, 6:56 PM
  • Add ATF C test for newly added acl_* functions.
Harbormaster completed remote builds in B36629: Diff 83159.Jan 30 2021, 6:56 PM
arrowd added a comment.EditedJan 30 2021, 7:01 PM

I've added an ATF test for new functions. For now, the issues are

  • I had to remove assert(_acl_brand(a) != ACL_BRAND_UNKNOWN) from acl_support.c. I don't quite see why these asserts were there in first place, but I need them removed to make use of _acl_differs in acl_cmp_np.
  • The acl_equiv_mode is very similar to our acl_is_trivial_np. I wonder if something should be done about this.
  • I plan to run the same ATF test on Linux to ensure identical semantics Done, test passes.
gbe added a subscriber: gbe.EditedApr 18 2021, 11:14 AM

Two small nits on the man pages, otherwise LGTM.

lib/libc/posix1e/acl_cmp_np.3
49

New sentence, new line.

lib/libc/posix1e/acl_equiv_mode_np.3
49

New sentence new line.

arrowd updated this revision to Diff 88533.May 3 2021, 11:10 AM
arrowd marked 3 inline comments as done.
  • Address comments.
  • tests/sys/acl: Add compatibility code for Linux.
  • libc/posix1e: Add acl_extended_file_np() function.
Harbormaster completed remote builds in B38979: Diff 88533.May 3 2021, 11:10 AM
arrowd updated this revision to Diff 93902.Aug 18 2021, 8:03 PM
  • Fix for silly error in acl_extended_file_np().
Harbormaster completed remote builds in B41119: Diff 93902.Aug 18 2021, 8:03 PM
arrowd added a reviewer: trasz.Aug 18 2021, 8:05 PM
kib added a subscriber: kib.Aug 18 2021, 8:11 PM
kib added inline comments.
lib/libc/posix1e/Symbol.map
88

1.7

lib/libc/posix1e/acl_cmp_np.c
47

Opening brace should be on the previous line.

Why do you check for NULL? This is untypical for libc interfaces.

53

return (1); style requires () around return value

lib/libc/posix1e/acl_equiv_mode_np.c
105

if (mode_p != NULL)

arrowd added inline comments.Aug 19 2021, 5:11 PM
lib/libc/posix1e/acl_cmp_np.c
47

No idea, to be honest. This is what Linux libacl does, so I just replicated its behavior. Should I remove it?

arrowd updated this revision to Diff 93947.Aug 19 2021, 5:51 PM
arrowd marked 3 inline comments as done.

Address comments.

Harbormaster completed remote builds in B41142: Diff 93947.Aug 19 2021, 5:51 PM
kib added inline comments.Aug 19 2021, 6:19 PM
lib/libc/posix1e/acl_cmp_np.c
47

If Linux does that ok. C code cannot validate a pointer, the check for NULL sometimes makes sense when API interprets it as absence of the value.

53

I did not marked all places with the style problems. Issues I pointed out are systematic. For instance the return on the next line needs (). And '{' the placement is not fixed, it is also everywhere.

tests/sys/acl/acl-api-test.c
28 ↗(On Diff #93947)

sys/param.h already includes sys/types.h

45 ↗(On Diff #93947)

static const?

78 ↗(On Diff #93947)

Blank line is needed after declaration block. Space is needed there: while (num_tests--)

arrowd updated this revision to Diff 93965.Aug 20 2021, 3:56 PM
arrowd marked 6 inline comments as done.
  • Thorough style(9) check.
  • Address comments.
Harbormaster completed remote builds in B41146: Diff 93965.Aug 20 2021, 3:56 PM
arrowd added inline comments.Aug 20 2021, 3:57 PM
lib/libc/posix1e/acl_cmp_np.c
53

Right. I feel so stupid about this, what I have been thinking? -_\
The style should be fine now.

kib added inline comments.Aug 20 2021, 9:09 PM
lib/libc/posix1e/acl_equiv_mode_np.c
63

Why not write this as a for() loop, to keep all control in single place?
Remove initialization of cur_entry, then just write

   for (cur_entry = 0; cur_entry < acl->ats_acl.acl_cnt; cur_entry++) {
       entry = &acl->ats_acl.acl_entry[cur_entry];
...
70

Move '{' to the previous line

78

Note that 'continue' is correct but somewhat non-idiomatic there. Wouldn't 'break' do the same?

lib/libc/posix1e/acl_extended_file_np.c
80 ↗(On Diff #93965)

Why acl_free() cannot be done right after the call to acl_is_trivial_np? I.e.

retval = acl_is_trivial_np(acl, &istrivial);
acl_free(acl);
if (retval == -1)
     return (-1);
return (!istrivial);
lib/libc/posix1e/acl_from_mode_np.c
57

should this be acl == NULL?

arrowd updated this revision to Diff 93996.Aug 21 2021, 11:24 AM
arrowd marked 5 inline comments as done.

Address comments.

Harbormaster completed remote builds in B41151: Diff 93996.Aug 21 2021, 11:24 AM
kib accepted this revision.Aug 21 2021, 12:14 PM
This revision is now accepted and ready to land.Aug 21 2021, 12:14 PM
Closed by commit rG19f7f91133af: libc/posix1e: Add acl_from_mode_np() function. (authored by arrowd). · Explain WhyAug 27 2021, 8:52 AM
This revision was automatically updated to reflect the committed changes.
arrowd added a commit: rG19f7f91133af: libc/posix1e: Add acl_from_mode_np() function..
arrowd added a commit: rG937f807a3675: libc/posix1e: Add acl_cmp_np() function..
arrowd added a commit: rGfcef0684f108: Fix build of bin/getfacl after libc changes..
arrowd added a commit: rGd81d5b2f2c9c: libc/posix1e: Add acl_equiv_mode_np() function..
arrowd added a commit: rGf8b88be850a5: tests/sys/acl: Add ATF C test for newly added acl_* functions..
arrowd added a commit: rGc468923b2210: libc/posix1e: Add acl_extended_file_np() function..

Revision Contents
Changeset List

PathSize
bin/
getfacl/
93 lines
lib/
libc/
posix1e/
6 lines
6 lines
84 lines
57 lines
96 lines
96 lines
97 lines
115 lines
sys/
sys/
5 lines

Diff 82647

View Options

bin/getfacl/getfacl.c

Loading...
View Options

lib/libc/posix1e/Makefile.inc

Loading...
View Options

lib/libc/posix1e/Symbol.map

Loading...
View Options

lib/libc/posix1e/acl_cmp_np.3

Loading...
View Options

lib/libc/posix1e/acl_cmp_np.c

Loading...
View Options

lib/libc/posix1e/acl_equiv_mode_np.3

Loading...
View Options

lib/libc/posix1e/acl_equiv_mode_np.c

Loading...
View Options

lib/libc/posix1e/acl_from_mode_np.3

Loading...
View Options

lib/libc/posix1e/acl_from_mode_np.c

Loading...
View Options

sys/sys/acl.h

Loading...