Details

Reviewers

ngie
rwatson
mckusick

Commits

rS298483: Add ATF tests for usr.sbin/extattr

Summary

Add ATF tests for the existing behavior of setextattr, rmextattr, lsextattr,
and getextattr.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3386
Build 3423: arc lint + arc unit

Event Timeline

asomers updated this revision to Diff 15023.Apr 8 2016, 8:27 PM

asomers retitled this revision from to Add ATF tests for usr.sbin/extattr.

asomers updated this object.

asomers edited the test plan for this revision. (Show Details)

asomers added a reviewer: rwatson.

asomers added subscribers: ken, ngie.

Herald added a subscriber: imp. · View Herald TranscriptApr 8 2016, 8:27 PM

Adding mckusick as a reviewer at the suggestion of phk. BTW, this test-only change will be followed up by a functional change to the extattr utilities.

I'll commit in the morning if nobody objects.

Doesn't this require kernel support?
Doesn't this require extattr support in the filesystem?
What filesystems has this been tested on? UFS? ZFS?

usr.sbin/extattr/tests/extattr_test.sh
2	Licensing tort is missing.
71	*NUL
207	*unprivileged

In D5889#128772, @ngie wrote:

Doesn't this require kernel support?

Already in place for about a decade.

Doesn't this require extattr support in the filesystem?

Both UFS and ZFS support it. Does the test suite need to run on any other file system?

What filesystems has this been tested on? UFS? ZFS?

UFS and ZFS.

usr.sbin/extattr/tests/extattr_test.sh
2	I'll fix.
71	I'll fix.
207	I'll fix.

In D5889#128782, @asomers wrote:

In D5889#128772, @ngie wrote:

Doesn't this require kernel support?

Already in place for about a decade.

Doesn't this require extattr support in the filesystem?

Both UFS and ZFS support it. Does the test suite need to run on any other file system?

What filesystems has this been tested on? UFS? ZFS?

UFS and ZFS.

extattr support can be compiled out of UFS at least by commenting out or not including UFS_EXTATTR. I'll put out another CR for adding this FEATURE knob.

Figuring out whether or not zfs has extattr enabled is something that needs to be done too, but it seems like there's no way to compile it out today. Can it be turned on/off somehow at the zfs(8) level?

In D5889#128791, @ngie wrote:

In D5889#128782, @asomers wrote:

In D5889#128772, @ngie wrote:

Doesn't this require kernel support?

Already in place for about a decade.

Doesn't this require extattr support in the filesystem?

Both UFS and ZFS support it. Does the test suite need to run on any other file system?

What filesystems has this been tested on? UFS? ZFS?

UFS and ZFS.

extattr support can be compiled out of UFS at least by commenting out or not including UFS_EXTATTR. I'll put out another CR for adding this FEATURE knob.

So then we'll be able to test for it with "require.config"? That would probably work, although it would still lead to skipping tests if it's compiled out of UFS even if /tmp is zfs.

Figuring out whether or not zfs has extattr enabled is something that needs to be done too, but it seems like there's no way to compile it out today. Can it be turned on/off somehow at the zfs(8) level?

On Illumos it can be controlled with the xattr filesystem property. But that doesn't work on FreeBSD. On FreeBSD, xattrs are always supported.

Add license and fix typos in extattr_test.sh

In D5889#128794, @asomers wrote:

So then we'll be able to test for it with "require.config"? That would probably work, although it would still lead to skipping tests if it's compiled out of UFS even if /tmp is zfs.

Sadly, it's not that nice. One of the things I'm going to push for this year (and hopefully before 11.0-RELEASE) is better requirements testing via require.<foo> with atf/kyua. Right now I've been doing things ad hoc around the tree... which is not ideal.

On Illumos it can be controlled with the xattr filesystem property. But that doesn't work on FreeBSD. On FreeBSD, xattrs are always supported.

Hmm. Ok.

ngie added inline comments.Apr 21 2016, 9:50 PM

usr.sbin/extattr/tests/extattr_test.sh

35–37

Would not-exit:0 be better than exit:255? This would ensure the exit code behavior isn't so strictly tested for, as it isn't documented in setextattr(8), and in case someone changes the exit codes (someday).

0x isn't required for explicitness with the ACL? This seems like a clunky interface...

58–60

My guess is that the copyin's failing because the in-kernel buffers are wrongly sized, i.e. they don't tack on another byte for a NUL (note how all of the buffers below are for EXTATTR_MAXNAMELEN, not EXTATTR_MAXNAMELEN+1):

$ (cd /usr/src/; grep -r EXTATTR_MAXNAMELEN sys/kern/)
sys/kern/vfs_extattr.c: char attrname[EXTATTR_MAXNAMELEN];
sys/kern/vfs_extattr.c:         error = copyinstr(uap->attrname, attrname, EXTATTR_MAXNAMELEN,
sys/kern/vfs_extattr.c: char attrname[EXTATTR_MAXNAMELEN];
sys/kern/vfs_extattr.c: error = copyinstr(uap->attrname, attrname, EXTATTR_MAXNAMELEN, NULL);
sys/kern/vfs_extattr.c: char attrname[EXTATTR_MAXNAMELEN];
sys/kern/vfs_extattr.c: error = copyinstr(uap->attrname, attrname, EXTATTR_MAXNAMELEN, NULL);
sys/kern/vfs_extattr.c: char attrname[EXTATTR_MAXNAMELEN];
sys/kern/vfs_extattr.c: error = copyinstr(uap->attrname, attrname, EXTATTR_MAXNAMELEN, NULL);
sys/kern/vfs_extattr.c: char attrname[EXTATTR_MAXNAMELEN];
sys/kern/vfs_extattr.c: error = copyinstr(uap->attrname, attrname, EXTATTR_MAXNAMELEN, NULL);
sys/kern/vfs_extattr.c: char attrname[EXTATTR_MAXNAMELEN];
sys/kern/vfs_extattr.c: error = copyinstr(uap->attrname, attrname, EXTATTR_MAXNAMELEN, NULL);
sys/kern/vfs_extattr.c: char attrname[EXTATTR_MAXNAMELEN];
sys/kern/vfs_extattr.c: error = copyinstr(uap->attrname, attrname, EXTATTR_MAXNAMELEN, NULL);
sys/kern/vfs_extattr.c: char attrname[EXTATTR_MAXNAMELEN];
sys/kern/vfs_extattr.c: error = copyinstr(uap->attrname, attrname, EXTATTR_MAXNAMELEN, NULL);
sys/kern/vfs_extattr.c: char attrname[EXTATTR_MAXNAMELEN];
sys/kern/vfs_extattr.c: error = copyinstr(uap->attrname, attrname, EXTATTR_MAXNAMELEN, NULL);
sys/kern/vfs_extattr.c: char attrname[EXTATTR_MAXNAMELEN];
sys/kern/vfs_extattr.c: error = copyinstr(uap->attrname, attrname, EXTATTR_MAXNAMELEN, NULL);
$

asomers added inline comments.Apr 21 2016, 10:05 PM

usr.sbin/extattr/tests/extattr_test.sh
35–37	It would be better. I'll fix.
47	You are confused because ABCD looks like a hex number. It is in fact a string. The "-x" only applies to getextattr. I'll change the test to use a less ambiguous attribute value.
58–60	Good guess. I'll open a PR to track it.

Open PR208965 for long_name, clarify the hex test, tolerate arbitrary attribute
order from lsextattr in the two attribute test, and fix a typo from my previous
spelling correction.

The tests seem fine minus the tab issue. I can't speak to their correctness *shrugs* (mckusick or rwatson might be better). I'll look over extattr and provide more feedback if they're busy though. We can deal with the ufs_extattr support check later.

usr.sbin/extattr/tests/Makefile
6	Empty line
usr.sbin/extattr/tests/extattr_test.sh
221–222	Are these hard tabs?

ngie added a reviewer: ngie.Apr 21 2016, 10:59 PM

ngie edited subscribers, added: tests; removed: ngie.

ngie added inline comments.

usr.sbin/extattr/tests/extattr_test.sh
221–222	Also, why is one or the other valid, but not both? Shouldn't both extattrs be set?

asomers added inline comments.Apr 21 2016, 11:05 PM

usr.sbin/extattr/tests/Makefile
6	Is it wrong to have one of those lines there? I tried to copy the style that I saw in various other makefiles, for example usr.sbin/newsyslog/tests/Makefile
usr.sbin/extattr/tests/extattr_test.sh
221–222	Yep, hard tabs is how lsextattr does it.
221–222	lsextattr only shows the attribute names, not the values. My comparison strings include both names.

ngie added inline comments.Apr 21 2016, 11:31 PM

usr.sbin/extattr/tests/Makefile
6	I'll go through and clean it out all in one-swoop...
usr.sbin/extattr/tests/extattr_test.sh
58–60	Thanks :)!!
221–222	Ok. Would sorting the output and/or using printf to produce the string be possible? Using printf would be preferred for readability. Sorting the output is less of an issue (but it would reduce the number of printf calls a bit).

Don't depend on lsextattr printing hard tabs

Enhancement: a lot of the tests explicitly use the -q flag with commands (to omit filenames); what about testing out the -q versions of commands?

usr.sbin/extattr/tests/extattr_test.sh
162–168	Enhancement: What about adding an attribute to the symlinked file instead and testing the inverse behavior?
209	Enhancement: it seems like a good testcase to verify that the rmextattr worked on bar ;).
221–222	Nice! That's a lot easier than the idea I had in mind :)...

Enhancement: would be a good idea to add a test that verifies that a file with system and user extended attributes works properly.

I reviewed the code and it LGTM. Everything marked "Enhancement:" can be done in a future commit. Thanks :)

This revision is now accepted and ready to land.Apr 22 2016, 12:23 AM

Closed by commit rS298483: Add ATF tests for usr.sbin/extattr (authored by asomers). · Explain WhyApr 22 2016, 5:03 PM

This revision was automatically updated to reflect the committed changes.

Add ATF tests for usr.sbin/extattr
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 15464

etc/mtree/BSD.tests.dist

usr.sbin/extattr/Makefile

usr.sbin/extattr/tests/

usr.sbin/extattr/tests/Makefile

usr.sbin/extattr/tests/extattr_test.sh

Add ATF tests for usr.sbin/extattrClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 15464

etc/mtree/BSD.tests.dist

usr.sbin/extattr/Makefile

usr.sbin/extattr/tests/

usr.sbin/extattr/tests/Makefile

usr.sbin/extattr/tests/extattr_test.sh

Add ATF tests for usr.sbin/extattr
ClosedPublic
Actions

Revision Contents
Changeset List