Re: [PATCH v6 1/3] xfstests: btrfs: add functions to create dm-error device

[Anand Jain <anand.jain@oracle.com>]

Dave,

On 08/21/2015 05:45 AM, Dave Chinner wrote:
> On Thu, Aug 20, 2015 at 03:09:05PM +0800, Anand Jain wrote:
>>
>>   (thanks for the off-ML emails from the people who helped me
>>   to understand).
>>
>> Dave,
>>
>>   looks like you are suggesting something like..
>>
>> -------
>> +_dmerror_mount_options()
>> +{
>> +       _scratch_options mount
>> +       echo $SCRATCH_OPTIONS $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS \
>> +                                       $* $DMERROR_DEV $SCRATCH_MNT
>> +}
>> +
>> +_dmerror_mount()
>> +{
>> +       _mount -t $FSTYP `_dmerror_mount_options $*`
>> +}
>> --------
>
> Very close. :)
>
> You've got the structure right, but missed one more thing: the
> options are still cut'n'paste from _scratch_mount_options, and so
> should be /factored/ into a common function...
>
> _scratch_options is only useful to XFS to set external log/realtime
> devices, and we don't want to do that for the dmerror device.
> Everthing else is almost the same as _scratch_mount_options(),
> however.  So:
>
> # Used for mounting non-scratch devices (e.g. loop, dm constructs)
> # with the safe set of scratch mount options (e.g. loop image may be
> # hosted on $SCRATCH_DEV, so can't use external scratch devices).
> _common_dev_mount_options()
> {
> 	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $*
> }
>
> _scratch_mount_options()
> {
> 	_scratch_options mount
>
> 	echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
> 					    $SCRATCH_DEV $SCRATCH_MNT
>
> }


> _dmerror_mount_options()
> {
>         echo `_common_dev_mount_options $*` $DMERROR_DEV $SCRATCH_MNT
> }


   this would make test cases nice and sleek. Thanks. I have this in v8.


> And to demonstrate why factoring like this is useful, this can also
> be applied to _mount_flakey(), and we could also replace most of the
> open-coded loop device mounts with a generic "mount device" function
> like:

   yes indeed. I suggest if these could be handled in a separate patch
   as this would deviate from the intentions of this patch set ?

Thanks, Anand


> _mount_dev()
> {
> 	_mount `_common_dev_mount_options $*`
> }
>
> and call it like:
>
> _mount_dev $LOOP_DEV $MNT_POINT
>
> or even
>
> _mount_dev -o loop $MNT_POINT
>
> We can then take it further: if we have a _mount_dev() wrapper we
> can factor _common_dev_mount_options() from all the
> _*_mount_options() functions. i.e:
>
> _scratch_mount_options()
> {
> 	_scratch_options mount
>
> 	echo $SCRATCH_OPTIONS $SCRATCH_DEV $SCRATCH_MNT
> }
>
> _scratch_mount()
> {
> 	_mount_dev $* `_scratch_mount_options`
> }
>
> The result is we end up with a much cleaner set of mount functions
> that are more generic, more extensible and easier to use, as well as
> being more maintainable....
>
>>   Sorry that the word 'should mirror _scratch_mount()' confused me.
>
> Ok, I'll try to be more clear in future.
>
> Cheers,
>
> Dave.
>