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@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
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
> > >
> >
>
