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

From: Filipe Manana <fdmanana@kernel.org>
To: Anand Jain <anand.jain@oracle.com>
Cc: fstests@vger.kernel.org, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 09/12] btrfs: introduce helper for creating cloned devices with mkfs
Date: Thu, 15 Feb 2024 12:42:26 +0000	[thread overview]
Message-ID: <CAL3q7H5fp8=SBeAbkmaJahTYCZQ5LQtbkcXR_2d1tXmhJ_Q87A@mail.gmail.com> (raw)
In-Reply-To: <b6026821942d5898dfc5f60d7a7c2b19574f764f.1707969354.git.anand.jain@oracle.com>

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