diff mbox series

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

Message ID b6026821942d5898dfc5f60d7a7c2b19574f764f.1707969354.git.anand.jain@oracle.com (mailing list archive)
State New, archived
Headers show
Series btrfs: functional test cases for tempfsid | expand

Commit Message

Anand Jain Feb. 15, 2024, 6:34 a.m. UTC
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(-)

Comments

Filipe Manana Feb. 15, 2024, 12:42 p.m. UTC | #1
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?

>         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".

> +
> +       _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.

Thanks.

> +
> +       _mkfs_dev -fq --uuid $fsid --device-uuid $uuid $dev2
> +}
> --
> 2.39.3
>
>
Anand Jain Feb. 17, 2024, 4:31 a.m. UTC | #2
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
>>
>>
diff mbox series

Patch

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
 	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"
+
+	_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}')
+
+	_mkfs_dev -fq --uuid $fsid --device-uuid $uuid $dev2
+}