On Sun, Jul 31, 2022 at 01:29:12PM +0800, Zorro Lang wrote: > On Thu, Jul 28, 2022 at 01:16:28PM -0700, Christoph Hellwig wrote: > > On Thu, Jul 28, 2022 at 11:17:15AM -0700, Darrick J. Wong wrote: > > > +SCRATCH_DEV=$metadump_img _scratch_xfs_repair -n &>> $seqres.full || \ > > > + echo "xfs_repair on restored fs returned $?" > > > > Wouldn;t it make more sense to have a version of _scratch_xfs_repair > > rather than doing a somewhat unexpected override of this global > > variable? > > Any detailed ideas about how to have a new version of _scratch_xfs_repair? I'll > keep this patch unmerged this week, before Darrick reply your discussion. > > Currently a few cases do some overriding [1] before calling _scratch_* helpers. > And good news is this kind of "override" affect only the environment seen by > its follow command/function. So I generally don't have objection if it works > well. But it's welcome if we have a better idea to deal with this kind of > requirement :) I don't know of a better way to do "xfs_repair the scratch filesystem, but with a different scratch device". One could create a new helper wherein any parameter that happens to be a path to a regular file or bdev would cause SCRATCH_DEV not to be included, but now we're walking arguments instead of being agnostic about them and merely passing them through to $XFS_REPAIR_PROG untouched. Worse yet, if there just happens to be (say) a file named '-d' or 'rmapbt=1' in the current directory, the test will think it should omit SCRATCH_DEV. The C getopt parser in xfs_repair won't see things this way, and these two tests become brittle over that. This can be worked around by parsing the arguments so that any getopt arguments will not be subjected to the "Should we override SCRATCH_DEV?" test. Because xfs_repair options change over time, we'd have to scan the binary to extract the getopt string (or construct it from --help). This is nontrivial, so the xfs_repair getopt extraction routines themselves will need a new fstest to test the test code, lest the two tests become brittle over that. All that fugly parsing crap can be worked around by teaching xfs_repair to ignore any respecified data device paths, but this is likely to cause resistance from the xfsprogs maintainers because that sounds like an avenue to introduce confusing behavior to end users. *OR* we could just override the variable definition for the one line since (for once) bash makes this easy and the syntax communicates very loudly "HI USE THIS ALT SCRATCH_DEV PLZ". And test authors already do this. --D > Thanks, > Zorro > > > [1] > $ grep -rsn SCRATCH_DEV= tests/ > tests/btrfs/160:36:old_SCRATCH_DEV=$SCRATCH_DEV > tests/btrfs/160:38:SCRATCH_DEV=$DMERROR_DEV > tests/btrfs/146:39:old_SCRATCH_DEV=$SCRATCH_DEV > tests/btrfs/146:41:SCRATCH_DEV=$DMERROR_DEV > tests/btrfs/146:62:SCRATCH_DEV=$old_SCRATCH_DEV > tests/xfs/507:198:LARGE_SCRATCH_DEV=yes _check_xfs_filesystem $loop_dev none none > tests/xfs/185:75:SCRATCH_DEV="$ddloop" > tests/xfs/234:56:SCRATCH_DEV=$TEST_DIR/image _scratch_mount > tests/xfs/234:57:SCRATCH_DEV=$TEST_DIR/image _scratch_unmount > tests/xfs/336:68:SCRATCH_DEV=$TEST_DIR/image _scratch_mount > tests/xfs/336:69:SCRATCH_DEV=$TEST_DIR/image _scratch_unmount > tests/xfs/157:61: SCRATCH_DEV=$orig_ddev > tests/xfs/157:76:SCRATCH_DEV=$fake_datafile > tests/xfs/129:56:SCRATCH_DEV=$TEST_DIR/image _scratch_mount > tests/xfs/129:57:SCRATCH_DEV=$TEST_DIR/image _scratch_unmount > tests/ceph/005:27:SCRATCH_DEV="$SCRATCH_DEV/quota-dir" _scratch_mount > tests/ceph/005:29:SCRATCH_DEV="$SCRATCH_DEV_ORIG/quota-dir" _scratch_unmount > tests/ceph/005:31:SCRATCH_DEV="$SCRATCH_DEV_ORIG/quota-dir/subdir" _scratch_mount > tests/ceph/005:33:SCRATCH_DEV="$SCRATCH_DEV_ORIG/quota-dir/subdir" _scratch_unmount > > > >
