This is an archive of the discontinued LLVM Phabricator instance.

Don't test setting sticky bits on files for (Free|Open)BSD
ClosedPublic

Authored by dim on Apr 16 2017, 6:36 AM.

Details

Summary

In rL297945, jhenderson added methods for setting permissions to
sys::fs, but some of the unittests that attempt to set sticky bits
(01000) on files fail on FreeBSD and OpenBSD. This is because those
systems do not allow regular users to set sticky bits on files, only on
directories. Fix it by disabling these particular tests on FreeBSD and
OpenBSD.

Event Timeline

dim created this revision.Apr 16 2017, 6:36 AM
jhenderson edited edge metadata.Apr 18 2017, 1:27 AM

Thanks for looking at this!

unittests/Support/Path.cpp
1539

Could you add an additional test that uses fs::all_perms, but without the sticky bit, please, so that we don't lose coverage of that enum value on FreeBSD? It wouldn't need to be inside the #if block. Something like the below:

EXPECT_EQ(fs::setPermissions(TempPath, fs::all_perms & ~fs::sticky_bit), NoError);
EXPECT_TRUE(CheckPermissions(fs::all_perms & ~fs::sticky_bit));
dim updated this revision to Diff 96280.Apr 22 2017, 4:39 AM

Add additional test for fs::all_perms without the sticky bit.

This is valid for NetBSD as well.

dim updated this revision to Diff 96284.Apr 22 2017, 7:57 AM

Also add NetBSD. At some point, we might want to have a global
LLVM_ON_BSD define to make these easier. :)

In D32120#734601, @dim wrote:

Also add NetBSD. At some point, we might want to have a global
LLVM_ON_BSD define to make these easier. :)

I don't agree. It would mean that it's supposed to work on BSD4.4 and derivate systems like BSD/OS.

emaste edited edge metadata.Apr 22 2017, 11:35 AM

I don't agree. It would mean that it's supposed to work on BSD4.4 and derivate systems like BSD/OS.

I think it would be useful to have a #define for modern BSDs though, as presumably DragonFly, bitrig, EdgeBSD etc. will need it set as well (although some/all may still include the #define inherited from their parent).

unittests/Support/Path.cpp
1518

Maybe just "Modern BSDs require..." so that the comment doesn't repeat the test and need to be updated if/when another one is added?

I don't agree. It would mean that it's supposed to work on BSD4.4 and derivate systems like BSD/OS.

I think it would be useful to have a #define for modern BSDs though, as presumably DragonFly, bitrig, EdgeBSD etc. will need it set as well (although some/all may still include the #define inherited from their parent).

I support automatic detection of features in configure time rather than ifdefing. Haiku, Minix, Darwin are close to BSD and they perhaps shouldn't be in the category of BSD, while they share similar interfaces.

joerg added a subscriber: joerg.Apr 22 2017, 12:09 PM

I think a better idea would be to just allow this tests to fail with EPERM.

dim added a comment.Apr 22 2017, 12:24 PM

I think a better idea would be to just allow this tests to fail with EPERM.

They resulted in EFTYPE though, not EPERM. I'm fine with either approach, though if you want to allow them to fail, the CheckPermissions calls also need to be conditional.

jhenderson accepted this revision.Apr 24 2017, 1:08 AM

LGTM, with the suggested comment change by @emaste.

I'm afraid I don't know enough about the BSD world to really be able to contribute to the rest of the discussion, but it feels like it's somewhat tangential to this patch, so maybe worth bringing up on the mailing lists? FWIW, if there are more BSD cases that this applies to, I'd be in favour of some sort of combined macro as suggested.

This revision is now accepted and ready to land.Apr 24 2017, 1:08 AM
This revision was automatically updated to reflect the committed changes.