On Mon, Oct 30, 2023 at 3:29 AM Anand Jain <anand.jain@xxxxxxxxxx> wrote:
>
>
>
>
> >> +
> >> + # 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.
Is it really worth doing this type of change?
I mean it doesn't change the correctness of the test, doesn't make it
more readable or
maintainable, or even shorter... It seems pointless to me, no clear
benefit of any sort.
>
>
> >> + $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'm not suggesting a new test case.
Remember the code you removed in v1?
My suggestion was to instead of removing it, just surround it in the body of an
if statement:
if temp-fsid-feature-not-abailable; then
run that code you tried to remove in v1
fi
Isn't that a lot simpler and clear?
Thanks.
>
> I will await your response before sending v3. Thx.
>
> Thanks, Anand
>
