Add ATF tests for the existing behavior of setextattr, rmextattr, lsextattr,
and getextattr.
Details
Details
- Reviewers
ngie rwatson mckusick - Commits
- rS298483: Add ATF tests for usr.sbin/extattr
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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.
Comment Actions
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 | ||
|---|---|---|
| 1 ↗ | (On Diff #15023) | I'll fix. |
| 70 ↗ | (On Diff #15023) | I'll fix. |
| 206 ↗ | (On Diff #15023) | I'll fix. |
Comment Actions
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?
Comment Actions
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.
Comment Actions
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.
| usr.sbin/extattr/tests/extattr_test.sh | ||
|---|---|---|
| 34–36 ↗ | (On Diff #15462) | 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). |
| 46 ↗ | (On Diff #15462) | 0x isn't required for explicitness with the ACL? This seems like a clunky interface... |
| 57–59 ↗ | (On Diff #15462) | 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); $ |
| usr.sbin/extattr/tests/extattr_test.sh | ||
|---|---|---|
| 34–36 ↗ | (On Diff #15462) | It would be better. I'll fix. |
| 46 ↗ | (On Diff #15462) | 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. |
| 57–59 ↗ | (On Diff #15462) | Good guess. I'll open a PR to track it. |
Comment Actions
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.
Comment Actions
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 ↗ | (On Diff #15464) | Empty line |
| usr.sbin/extattr/tests/extattr_test.sh | ||
| 221–222 ↗ | (On Diff #15464) | Are these hard tabs? |
| usr.sbin/extattr/tests/extattr_test.sh | ||
|---|---|---|
| 221–222 ↗ | (On Diff #15464) | Also, why is one or the other valid, but not both? Shouldn't both extattrs be set? |
| usr.sbin/extattr/tests/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #15464) | 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 ↗ | (On Diff #15464) | Yep, hard tabs is how lsextattr does it. |
| 221–222 ↗ | (On Diff #15464) | lsextattr only shows the attribute names, not the values. My comparison strings include both names. |
| usr.sbin/extattr/tests/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #15464) | I'll go through and clean it out all in one-swoop... |
| usr.sbin/extattr/tests/extattr_test.sh | ||
| 221–222 ↗ | (On Diff #15464) | 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). |
| 57–59 ↗ | (On Diff #15462) | Thanks :)!! |
Comment Actions
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 | ||
|---|---|---|
| 161–167 ↗ | (On Diff #15465) | Enhancement: What about adding an attribute to the symlinked file instead and testing the inverse behavior? |
| 208 ↗ | (On Diff #15465) | Enhancement: it seems like a good testcase to verify that the rmextattr worked on bar ;). |
| 221–222 ↗ | (On Diff #15464) | Nice! That's a lot easier than the idea I had in mind :)... |
Comment Actions
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 :)