Re: [PATCH 04/12] btrfs: create a helper function, check_fsid(), to verify the tempfsid

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]



On Thu, Feb 15, 2024 at 6:35 AM Anand Jain <anand.jain@xxxxxxxxxx> wrote:
>
> check_fsid() provides a method to verify if the given device is mounted
> with the tempfsid in the kernel. Function sb() is an internal only
> function.
>
> Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
> ---
>  common/btrfs | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/common/btrfs b/common/btrfs
> index e1b29c613767..5cba9b16b4de 100644
> --- a/common/btrfs
> +++ b/common/btrfs
> @@ -792,3 +792,37 @@ _has_btrfs_sysfs_feature_attr()
>
>         test -e /sys/fs/btrfs/features/$feature_attr
>  }
> +
> +# Dump key members of the on-disk super-block from the given disk; helps debug
> +sb()
> +{
> +       local dev1=$1
> +       local parameters="device|devid|^metadata_uuid|^fsid|^incom|^generation|\
> +               ^flags| \|$| \)$|compat_flags|compat_ro_flags|dev_item.uuid"
> +
> +       $BTRFS_UTIL_PROG inspect-internal dump-super $dev1 | egrep "$parameters"
> +}

Don't add such a short function name that doesn't minimally indicate
what it does.... especially taking into account that it's polluting
the global namespace and isn't used anywhere else.
So this code should be in the function below, as it's only used once....

> +
> +check_fsid()
> +{
> +       local dev1=$1
> +       local fsid
> +
> +       # on disk fsid
> +       fsid=$(sb $dev1 | grep ^fsid | awk -d" " '{print $2}')

Please use $AWK_PROG instead.

Again this sb() function is pointless, even because here we only care
about the fsid line, yet the function is extracting a lot of other
lines without any users.

> +       echo -e "On disk fsid:\t\t$fsid" | sed -e "s/$fsid/FSID/g"
> +
> +       echo -e -n "Metadata uuid:\t\t"
> +       cat /sys/fs/btrfs/$fsid/metadata_uuid | sed -e "s/$fsid/FSID/g"

Ok, so why do we care about printing the fsid and metadata uuid lines
if we always replace the IDs with the constant FSID?
It seems pointless to print them... At the very least this deserves a
comment, a rationale on what this accomplishes.

Thanks.

> +
> +       # This returns the temp_fsid if set
> +       tempfsid=$(_btrfs_get_fsid $dev1)
> +       if [[ $tempfsid == $fsid ]]; then
> +               echo -e "Temp fsid:\t\tFSID"
> +       else
> +               echo -e "Temp fsid:\t\tTEMPFSID"
> +       fi
> +
> +       echo -e -n "Tempfsid status:\t"
> +       cat /sys/fs/btrfs/$tempfsid/temp_fsid
> +}
> --
> 2.39.3
>
>





[Index of Archives]     [Linux Filesystems Development]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux