Details
- Reviewers
lwhsu mckusick - Group Reviewers
tests - Commits
- rG55141f2c8991: Add tests for gunion(8)
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
The tests are described below.
- load:
- Use kldstat to confirm the gunion load and unload is actually work.
- create:
- Look /dev/ folder to confirm the gunion create and destroy is actually work.
- commit:
- Use checksum to check the file is actually commit.
- offset:
- Create two partitions and see if we can mount the union device which use the two partitions. Test the gunion commit -o offset.
- size:
- Use diskinfo test gunion commit -s size.
- secsize:
- Use diskinfo test gunion commit -S secsize.
- gunionname:
- Look /dev/ folder to test gunion commit -Z gunionname.
- revert:
- Use checksum to check the file is actually revert.
I am happy to see a set of gunion tests being added. The set of tests looks complete.
In general, only the lower (readable) disk partition is formatted with a filesystem. The upper (writable) layer is used and managed by gunion. It does not cause any errors for you to format it as a filesystem, but it is not necessary to do so. Attempting to access the top layer as a filesystem after it has been used by gunion is not always going to work as gunion makes no effort to keep it consistent. In particular if you use the -o offset option, any filesystem built on the top layer will cease to be usable after gunion uses it as a top layer.
Have you verified that these tests work?
I have verified the test and it works (make install the test and run kyua test in the test directory, and I also run these commands manually to verify). But as you say, in general, the upper layer is not necessary to format the file system. I will change the test to not do this. Thanks for the review!
Format writable_device statement remove. In general, upper device is not necessary to format.
Your checksum test is a good way to verify that everything worked as expected though it is possible that some sort of filesystem corruption snuck in (obviously to both copies). So you might want to add a second check by running 'fsck -p -f' on the both the gunion filesystem and the original filesystem after the gunion has been dissolved. The exit status from fsck should be zero.
Use fsck -p -f to check gunion and readable device
- Use fsck -p -f to check the gunion device before it is destroyed in create and offset test cases.
- Check the readable device (lower device) after it is destroyed in commit test case.
I don't check the writable device (upper device) because as above say, gunion makes no effort to keep the upper device file system.
Sorry it takes me longer to review the code. Some suggestions:
- We need atf_set "require.user" "root" in all _head() sections as those test cases have to be run by root.
- Should we change the name of writable_device and readable_device to upper_device and lower_device or even upperdev and lowerdev, as the description in manual page? After all, we will still write (commit) to the readable_device so this name might be confusing.
- Should we check the original file in the readable (lower) device is unmodified after gunion is destroyed? maybe this can be a "basic" case?
- Should we have a test case to check the readable (lower) device cannout be mounted when it's in a gunion?
Other comments specified to individual test cases will be put inline.
| tests/sys/geom/class/union/conf.sh | ||
|---|---|---|
| 29 | Is _1 necessary? | |
| tests/sys/geom/class/union/union_test.sh | ||
| 99 | In this "commit" test case, we do mount the writable (upper) device and check the content on it. I am not sure if this behavior is guaranteed to work and necessary. i.e., this section: Will these be enough? atf_check -o ignore fsck -p -f "/dev/${readable_device_name}"
mount "/dev/${readable_device_name}" readable_device
checksum readable_device/readable_file readable_file
checksum readable_device/gunion_file gunion_file | |
| 118 | Is this needed for the upper layer? | |
| 120 | Is this needed for the upper layer? | |
| 126 | Would gpt_sector_count be better as it's describing how many sectors it has? | |
| 135 | Finally, should we verify the gpt metadata is not modified (means that the offset option works)? | |
| 178 | Should we specify -S 512 in newfs of the readable (lower) device, to make sure that the gunion's secsize override really work? | |
| 247 | Is this line guaranteed to work? | |
| 249 | Is this line necessary? | |
| 250 | Finally, should we check if "gunion_file" is not shown in readable (lower) device after reverting? | |
| tests/sys/geom/class/union/union_test.sh | ||
|---|---|---|
| 99 | Is the gunion guarantee this behavior? gunion read data from "readable_device" and write the data to "writable_device". So I write the codes to check the gunion actually write the data and the data is same as the original one. | |
| 135 | To make sure what I think. This is for checking the "readable_device" GPT entry right? "writable_device" will not have GPT entry because we use the "offset" option. By the way, I use the mount to check the offset is actually work. Because if we don't use "offset" options to skip the GPT entry, we can't mount the "gunion_device". | |
- Update according lwhsu advices
- Add "basic" case to test the "lowerdev" is umnodified after gunion is destroyed and test "lowerdev" can not be mounted when it's in gunion.
- Add atf_set to set root requirement.
- Change the variables name like "writable_device" to "upperdev", "readable_device" to "lowerdev", "gunion_device" to "guniondev" ...
- Add atf_check_equal in "secsize" test case to check the lowerdev secsize is actually 512 becuse this is very important for the remain tests
| tests/sys/geom/class/union/conf.sh | ||
|---|---|---|
| 33 | It's better not unload for others. Although it's better to minimize the side effect, but unloading unconditionally might result that someone already have that module loaded got unloaded after running tests. I think it's worse than having a module loaded as leftover. Or we have to check if the module is loaded before loading, then restore the original status. That's what we did for some sysctls, but I feel implementing it here might cause unnecessary complexity. | |
| tests/sys/geom/class/union/union_test.sh | ||
| 57 | Have a comment for this line to describe it is used to check if lower device cannot be mounted when being in a gunion. Or put to the atf_set "descr" section. | |
| 58 | Before destory gunion, it's better to write something to lower_file in the gunion, or the file will never be touched in the upper layer, thus we cannot be sure it won't be affected. | |
| 68 | I suggest moving "create" test to the first one as it's the simplest one now. "basic" is doing more things. | |
| 196 | have a comment about how to calculate the magic number: 2097152 | |
| 287 | I think what we actually want to check is the gunion is reverted to the original state as lower device. So what we really want to check is the state of the mounted gunion (after umount and revert). Or if we check the lower device, it's the same with what we tested in the "basic" case. | |
| 289 | minor nit: extra white space in not-exit:0 -o | |
- Update according lwhsu advices
- Remove gunion unload.
- Update "basic" test case.
- Move "create" teas case to the first one.
- Describe "2097152" magic number.
- Update "revert" test case.
- Revise "lower_file" in the "commit" case.
I revise the original "lower_file" and "lower_file" in "guniondev". Then after committing, use the checksum to check the two files are the same. I am not sure if this is OK.
| tests/sys/geom/class/union/union_test.sh | ||
|---|---|---|
| 132 | I suggest replacing this line with cp -f lower_file gunionmnt/lower_file to ensure the two files are identical, and keeps this meaning in semantic. | |