Re: [PATCH 2/2 v2] fstests: btrfs/219 cloned-device mount capability update

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]






+
+       # The variables are set before the test case can fail.

What if the _require_* statements fail?
Then the variables won't be defined...

Oh, yes indeed. It was fixed in v3 by moving the variable definition
before the 'require' statement.


+       $UMOUNT_PROG ${loop_mnt1} &> /dev/null
+       $UMOUNT_PROG ${loop_mnt2} &> /dev/null
+
+       _destroy_loop_device $loop_dev1 &> /dev/null
+       _destroy_loop_device $loop_dev2 &> /dev/null
+
+       rm -rf $fs_img1 &> /dev/null
+       rm -rf $fs_img2 &> /dev/null
+
+       rm -rf $loop_mnt1 &> /dev/null
+       rm -rf $loop_mnt2 &> /dev/null

Also please for simplicity and clarity don't mix this type of change
with the actual purpose of the patch,
to make the test succeed on a kernel with the temp-fsid feature.

You're mixing 3 different changes in the same patch...

-loop_mnt=$TEST_DIR/$seq.mnt
-loop_mnt1=$TEST_DIR/$seq.mnt1
-fs_img1=$TEST_DIR/$seq.img1
-fs_img2=$TEST_DIR/$seq.img2
+loop_mnt1=$TEST_DIR/$seq/mnt1
+loop_mnt2=$TEST_DIR/$seq/mnt2
+fs_img1=$TEST_DIR/$seq/img1
+fs_img2=$TEST_DIR/$seq/img2

So this is the other unrelated change, renaming all these variables...
This is making the diff larger to follow as this has nothing to do
with the goal of making the test succeed on a kernel with the
temp-fsid feature.


Sure. I'll move these changes to a new patch in v3.


+_mount $loop_dev2 $loop_mnt2 > /dev/null 2>&1
+
+if [ $? == 0 ]; then
+       # On temp-fsid supported kernels, mounting cloned device will be successfull.
+       if _has_fs_sysfs_attr $loop_dev2 temp_fsid; then
+               temp_fsid=$(_get_fs_sysfs_attr ${loop_dev2} temp_fsid)
+               if [ $temp_fsid == 0 ]; then
+                       _fail "Cloned devices mounted without temp_fsid"
+               fi


Hmm. It is exactly testing what happens with and without the temp-fsid feature.

This is too complex. Why not just surround the existing code in an if
statement like this:

if "sysfs-file-for-temp-fsid  does not exist" then
      run this code that fails with a temp-fsid enabled kernel
fi


Your suggestion will test what to expect when there is no 'temp-fsid.'

However, if 'temp-fsid' is present, we expect its sysfs temp_fsid
to be set to 1, which I believe is a straightforward verification.

And do you think adding more comments will make it simpler?
Such as:
# If the second mount is successful, then check if 'temp-fsid'
# is in the kernel. If it's present, verify that it outputs 1.

Or, Are you suggesting we postpone confirming the correct operation of
'temp-fsid' for another test case? However, I believe in thoroughly
verifying it wherever it's used, as this approach promotes a better
understanding of what to anticipate. No?

I will await your response before sending v3. Thx.

Thanks, Anand





[Index of Archives]     [Linux Filesystems Development]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux