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@xxxxxxxxxx>
> > ---
>
> 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
> > 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 ]
$ 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
> > + 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?
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
> >
>
