Add some more non-portable acl_* functions.

[arrowd]

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.

[debdrup]

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

lib/libc/posix1e/acl_cmp_np.3

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.

lib/libc/posix1e/acl_equiv_mode_np.3

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.

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]

• Address comments on man pages.

[arrowd]

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

53–54 ↗

(On Diff #82637)

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]

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]

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

[arrowd]

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]

Two small nits on the man pages, otherwise LGTM.

lib/libc/posix1e/acl_cmp_np.3

48 ↗

(On Diff #83159)

New sentence, new line.

lib/libc/posix1e/acl_equiv_mode_np.3

48 ↗

(On Diff #83159)

New sentence new line.

[arrowd]

• Address comments.
• tests/sys/acl: Add compatibility code for Linux.
• libc/posix1e: Add acl_extended_file_np() function.

[arrowd]

• Fix for silly error in acl_extended_file_np().

[kib]

lib/libc/posix1e/Symbol.map

88

1.7

lib/libc/posix1e/acl_cmp_np.c

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

lib/libc/posix1e/acl_equiv_mode_np.c

104 ↗

(On Diff #93902)

if (mode_p != NULL)

[arrowd]

lib/libc/posix1e/acl_cmp_np.c

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]

Address comments.

[kib]

lib/libc/posix1e/acl_cmp_np.c

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.

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]

• Thorough style(9) check.
• Address comments.

[arrowd]

lib/libc/posix1e/acl_cmp_np.c

52 ↗

(On Diff #93902)

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

[kib]

lib/libc/posix1e/acl_equiv_mode_np.c

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?

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]

Address comments.