Re: [LTP] [PATCH 0/2] Fix tst_find_backing_dev adding BTRFS support and /dev/root missing case

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Alessandro Carminati <alessandro.carminati@gmail.com> writes:

> Hello Richard,
>
> Thanks for the detailed review.
> If in the future I want to contribute more to the LTP project, I need
> to provide myself with a CI pipeline like yours.

If you fork the project in Github and create a new branch then the CI
will run on the commits you push to GH.

> I appreciated the review that was very detailed, but I couldn't address
> a single comment.
>
>>> +		if (!ioctl(fd, BTRFS_IOC_FS_INFO, &args)) {
>>What happens if the test author allows this function to be called on
> tmpfs, rootfs, etc.? Or if the FS is valid, but has the same issue as
> BTRFS.
>
> I have gone thorough all the file systems supported by LTP at this stage,
> and I noticed that BTRFS is the only file system that owns this
> singularity.

tmpfs doesn't have a backing device and we support it? E.g.

40 60 0:38 / /tmp rw,nosuid,nodev shared:19 - tmpfs tmpfs rw,nr_inodes=1048576,inode64

So this function shouldn't be called on it and it is not in the test
currently effected[1]. However if the test author does it by accident (99%
chance of happening) then we want a sensible error message.

> In addition to this, I also dared to assume that if device major number
> is == 0 then the test is facing the BTRFS.
> This assumption might not be true in general, but I tested it to be
> true in the test supported file system.
> Is your comment referring to this?

I think it is absolutely correct that we shouldn't design for
requirements we presently don't have. However:

1. It's highly probable this function will be misused.
2. It's relatively easy to guard against.

>
>
> Alessandro Carminati (2):
>   tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent
>   c-test-api: Documentation updated
>
>  doc/c-test-api.txt |  7 ++--
>  lib/tst_device.c   | 87 ++++++++++++++++++++++++++++++----------------
>  2 files changed, 63 insertions(+), 31 deletions(-)

[1]: The test skips it

.skip_filesystems = (const char *const []) {
	"tmpfs",
	"overlayfs",
	NULL
},

Also note that you can run LTP on any filesystem. You just need to set
the appropriate env vars.


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp