Re: [PATCH 09/12] btrfs: introduce helper for creating cloned devices with mkfs

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

On 2/15/24 18:12, Filipe Manana wrote:
> On Thu, Feb 15, 2024 at 6:37 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> Use newer mkfs.btrfs option to generate two cloned devices,
>> used in test cases.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   common/btrfs | 24 ++++++++++++++++++------
>>   1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/common/btrfs b/common/btrfs
>> index 9a7fa2c71ec5..8ffce3c39695 100644
>> --- a/common/btrfs
>> +++ b/common/btrfs
>> @@ -91,13 +91,7 @@ _require_btrfs_mkfs_feature()
>>   _require_btrfs_mkfs_uuid_option()
>>   {
>>          local cnt
>> -       local feature
>>
>> -       if [ -z $1 ]; then
>> -               echo "Missing option name argument for _require_btrfs_mkfs_option"
>> -               exit 1
>> -       fi
>> -       feature=$1
> 
> This is confusing... It was just introduced in the previous patch that
> introduced this function (_require_btrfs_mkfs_uuid_option).
> 
> If there was never any need for this code, why was it added in the
> previous patch and removed in this one, without any users in between?
> 

This change was mistakenly introduced here; it should have been in
the previous patch. I will move it. Thx.

>>          cnt=$($MKFS_BTRFS_PROG --help 2>&1 |grep -E --count "\-\-uuid|\-\-device-uuid")
>>          if [ $cnt != 2 ]; then
>>                  _notrun "Require $MKFS_BTRFS_PROG with --uuid and --device-uuid option"
>> @@ -864,3 +858,21 @@ create_cloned_devices()
>>                                                          _fail "dd failed: $?"
>>          echo done
>>   }
>> +
>> +mkfs_clone()
>> +{
>> +       local dev1=$1
>> +       local dev2=$2
>> +
>> +       [[ -z $dev1 || -z $dev2 ]] && \
>> +               _fail "BUGGY code, mkfs_clone needs arg1 arg2"
> 
> Rather more clear and informative to say "mkfs_clone requires two
> devices as arguments".

Sure.

> 
>> +
>> +       _mkfs_dev -fq $dev1
>> +
>> +       fsid=$($BTRFS_UTIL_PROG inspect-internal dump-super $dev1 | \
>> +                                       grep -E ^fsid | $AWK_PROG '{print $2}')
>> +       uuid=$($BTRFS_UTIL_PROG inspect-internal dump-super $dev1 | \
>> +                               grep -E ^dev_item.uuid | $AWK_PROG '{print $2}')
> 
> Make the variables local please.
> 

Sure.

Thanks, Anand
> Thanks.
> 
>> +
>> +       _mkfs_dev -fq --uuid $fsid --device-uuid $uuid $dev2
>> +}
>> --
>> 2.39.3
>>
>>