Page MenuHomeFreeBSD

Add some more non-portable acl_* functions.

Authored by arrowd on Jan 20 2021, 3:55 PM.



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

R10 FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

53 ↗(On Diff #82637)

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

55 ↗(On Diff #82637)

Same as above.

26 ↗(On Diff #82637)

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

44 ↗(On Diff #82637)

You missed a word here.

46 ↗(On Diff #82637)

A bit of wordsmithing to help ledgibility

53–54 ↗(On Diff #82637)

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

56 ↗(On Diff #82637)

Missed a letter here.

67 ↗(On Diff #82637)

Same as above.


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.


'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?


Same as above.


Same as above.


Same as above.

arrowd marked 12 inline comments as done.
  • Address comments on man pages.

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

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

53–54 ↗(On Diff #82637)

The latter, thanks.


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

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.

  • Add ATF C test for newly added acl_* functions.

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.

Two small nits on the man pages, otherwise LGTM.

48 ↗(On Diff #83159)

New sentence, new line.

48 ↗(On Diff #83159)

New sentence new line.

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.
  • Fix for silly error in acl_extended_file_np().
kib added inline comments.


46 ↗(On Diff #93902)

Opening brace should be on the previous line.

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

52 ↗(On Diff #93902)

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

104 ↗(On Diff #93902)

if (mode_p != NULL)

46 ↗(On Diff #93902)

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

arrowd marked 3 inline comments as done.

Address comments.

46 ↗(On Diff #93902)

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.

52 ↗(On Diff #93902)

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.

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 marked 6 inline comments as done.
  • Thorough style(9) check.
  • Address comments.
52 ↗(On Diff #93902)

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

62 ↗(On Diff #93965)

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];
69 ↗(On Diff #93965)

Move '{' to the previous line

77 ↗(On Diff #93965)

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

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);
if (retval == -1)
     return (-1);
return (!istrivial);

should this be acl == NULL?

arrowd marked 5 inline comments as done.

Address comments.

This revision is now accepted and ready to land.Aug 21 2021, 12:14 PM