Re: [PATCH] fstests: add helper to canonicalize devices used to enable persistent disks

[Zorro Lang <zlang@redhat.com>]

On Tue, Jul 25, 2023 at 08:54:39AM -0700, Darrick J. Wong wrote:
> On Tue, Jul 25, 2023 at 04:13:07PM +0800, Zorro Lang wrote:
> > On Wed, Jul 19, 2023 at 11:17:27PM -0700, Luis Chamberlain wrote:
> > > The filesystem configuration file does not allow you to use symlinks to
> > > devices given the existing sanity checks verify that the target end
> > > device matches the source.
> > > 
> > > Using a symlink is desirable if you want to enable persistent tests
> > > across reboots. For example you may want to use /dev/disk/by-id/nvme-eui.*
> > > so to ensure that the same drives are used even after reboot. This
> > > is very useful if you are testing for example with a virtualized
> > > environment and are using PCIe passthrough with other qemu NVMe drives
> > > with one or many NVMe drives.
> > > 
> > > To enable support just add a helper to canonicalize devices prior to
> > > running the tests.
> > > 
> > > This allows one test runner, kdevops, which I just extended with
> > > support to use real NVMe drives. The drives it uses for the filesystem
> > > configuration optionally is with NVMe eui symlinks so to allow
> > > the same drives to be used over reboots.
> > > 
> > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > ---
> > 
> > Hi Luis,
> > 
> > Hmmm... this's a default behavior change for fstests, although I'm not sure
> > what will be affect. I'm wondering if we should do this in fstests. I don't
> > want to tell the users that the device names they give to fstests will always
> > be truned to real names from now on.
> > 
> > Generally the users of fstests provide the device names, so the users
> > might need to take care of the name is "/dev/mapper/testvg-testdev"
> > or "/dev/dm-4". The users can deal with the device names when their
> > script prepare to run fstests.
> > 
> > If more developers prefer this change, I'd like to make it to be
> > optional by an option of ./check at least, not always turn devname
> > to realpath. Welcome review points from others.
> 
> Hmm.  SCRATCH_DEV=/dev/testvg/testlv works right now, it'd be sort of
> annoying to have "/dev/dm-4" get written into the report and then you've
> lost the correlation to whatever the user passed in.
> 
> Oh wait.  My giant mound of ./check wrapper script already does that
> canonicalization and has for years.
> 
> Ok.  Sounds good to me then. :P

So you hope fstests can do this translation (use real device name) by default?

> 
> > >  check         |  1 +
> > >  common/config | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 44 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/check b/check
> > > index 89e7e7bf20df..d063d3f498fd 100755
> > > --- a/check
> > > +++ b/check
> > > @@ -734,6 +734,7 @@ function run_section()
> > >  	fi
> > >  
> > >  	get_next_config $section
> > > +	_canonicalize_devices
> > >  
> > >  	mkdir -p $RESULT_BASE
> > >  	if [ ! -d $RESULT_BASE ]; then
> > > diff --git a/common/config b/common/config
> > > index 936ac225f4b1..f5a3815a0435 100644
> > > --- a/common/config
> > > +++ b/common/config
> > > @@ -655,6 +655,47 @@ _canonicalize_mountpoint()
> > >  	echo "$parent/$base"
> > >  }
> > >  
> > > +# Enables usage of /dev/disk/by-id/ symlinks to persist target devices
> > > +# over reboots
> > > +_canonicalize_devices()
> > > +{
> > > +	if [ ! -z "$TEST_DEV" ] && [ -L $TEST_DEV ]; then
> > 
> > I think [ -L "$TEST_DEV" ] is enough.
> 
> I don't think it is.
> 
> $ unset moo
> $ [ -L $moo ]

The double quotation marks "" are needed.

> $ echo $?
> 0
> $ realpath -e $moo
> realpath: missing operand
> Try 'realpath --help' for more information.
> 
> > > +		TEST_DEV=$(realpath -e $TEST_DEV)
> > 
> > Anyone knows the difference of realpatch and readlink?
> 
> readlink doesn't follow nested symlinks; realpath does:
> 
> $ touch /tmp/a
> $ ln -s /tmp/a /tmp/b
> $ ln -s /tmp/b /tmp/c
> $ readlink /tmp/c
> /tmp/b
> $ realpath /tmp/c
> /tmp/a
> $ readlink -m /tmp/c
> /tmp/a

The -e option helps:

# readlink -e /tmp/c
/tmp/a
# realpath -e /tmp/c
/tmp/a

> 
> > > +	fi
> > > +
> > > +	if [ ! -z "$SCRATCH_DEV" ] && [ -L $SCRATCH_DEV ]; then
> > > +		SCRATCH_DEV=$(realpath -e $SCRATCH_DEV)
> 
> Extra question: Shouldn't we be putting theis ^^^ variables in quotes?

Make sense to me.

Thanks,
Zorro

> 
> Supposing someone starts passing in
> SCRATCH_DEV=/dev/disk/by-label/har har"
> 
> This expression will barf everywhere:
> 
> $ SCRATCH_DEV=$(realpath -e $SCRATCH_DEV)
> realpath: /dev/disk/by-label/har: No such file or directory
> realpath: har: No such file or directory
> 
> Due to the lack of quoting on its way to turning that into /dev/sda3.
> Granted fstests has historically required no spaces in anything, but
> still, it's bad hygiene.
> 
> [writing anything in bash is bad hygiene, but for the sweet sweet pipe
> goodness]
> 
> > > +	fi
> > > +
> > > +	if [ ! -z "$TEST_LOGDEV" ] && [ -L $TEST_LOGDEV ]; then
> > > +		TEST_LOGDEV=$(realpath -e $TEST_LOGDEV)
> > > +	fi
> > > +
> > > +	if [ ! -z "$TEST_RTDEV" ] && [ -L $TEST_RTDEV ]; then
> > > +		TEST_RTDEV=$(realpath -e $TEST_RTDEV)
> > > +	fi
> > > +
> > > +	if [ ! -z "$SCRATCH_RTDEV" ] && [ -L $SCRATCH_RTDEV ]; then
> > > +		SCRATCH_RTDEV=$(realpath -e $SCRATCH_RTDEV)
> > > +	fi
> > > +
> > > +	if [ ! -z "$LOGWRITES_DEV" ] && [ -L $LOGWRITES_DEV ]; then
> > > +		LOGWRITES_DEV=$(realpath -e $LOGWRITES_DEV)
> > > +	fi
> > > +
> > > +	if [ ! -z "$SCRATCH_DEV_POOL" ]; then
> > > +		NEW_SCRATCH_POOL=""
> > > +		for i in $SCRATCH_DEV_POOL; do
> > > +			if [ -L $i ]; then
> > > +				NEW_SCRATCH_POOL="$NEW_SCRATCH_POOL $(realpath -e $i)"
> > > +			else
> > > +				NEW_SCRATCH_POOL="$NEW_SCRATCH_POOL $i)"
> >                                                                      ^^^
> > 
> > What's this half ")" for ?
> 
> Some kind of typo, I assume...
> 
> --D
> 
> > 
> > Thanks,
> > Zorro
> > 
> > 
> > > +			fi
> > > +		done
> > > +		SCRATCH_DEV_POOL="$NEW_SCRATCH_POOL"
> > > +	fi
> > > +}
> > > +
> > >  # On check -overlay, for the non multi section config case, this
> > >  # function is called on every test, before init_rc().
> > >  # When SCRATCH/TEST_* vars are defined in config file, config file
> > > @@ -785,7 +826,6 @@ get_next_config() {
> > >  	fi
> > >  
> > >  	parse_config_section $1
> > > -
> > >  	if [ ! -z "$OLD_FSTYP" ] && [ $OLD_FSTYP != $FSTYP ]; then
> > >  		[ -z "$MOUNT_OPTIONS" ] && _mount_opts
> > >  		[ -z "$TEST_FS_MOUNT_OPTS" ] && _test_mount_opts
> > > @@ -901,5 +941,7 @@ else
> > >  	fi
> > >  fi
> > >  
> > > +_canonicalize_devices
> > > +
> > >  # make sure this script returns success
> > >  /bin/true
> > > -- 
> > > 2.39.2
> > > 
> > 
>