Re: [PATCH v4 1/6] common: refactor sysfs_attr functions

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

On 29/09/2023 07:16, Boris Burkov wrote:
> Expand the has/get/require functions to allow passing a dev by
> parameter, and implement the test_dev specific one in terms of the new
> generic one.
> 


> Signed-off-by: Boris Burkov <boris@bur.io>

looks good. The diff is confusing.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

a small nit below.

>   
> +# Test for the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ATTR
> +#
> +# All arguments are necessary, and in this order:
> +#  - dev: device name, e.g. $SCRATCH_DEV
> +#  - attr: path name under /sys/fs/$FSTYP/$dev
> +#
> +# Usage example:
> +#   _has_fs_sysfs_attr /dev/mapper/scratch-dev error/fail_at_unmount
> +_has_fs_sysfs_attr()
> +{
> +	local dev=$1
> +	local attr=$2
> +
> +	if [ ! -b "$dev" -o -z "$attr" ];then
> +		_fail "Usage: _get_fs_sysfs_attr <mounted_device> <attr>"
                               ^^^
				_has_fs_sysfs_attr

No need to resend.

Thanks, Anand


> +	fi
> +
> +	local dname=$(_fs_sysfs_dname $dev)
> +
> +	test -e /sys/fs/${FSTYP}/${dname}/${attr}
> +}
> +
> +# Require the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ATTR
> +# All arguments are necessary, and in this order:
> +#  - dev: device name, e.g. $SCRATCH_DEV
> +#  - attr: path name under /sys/fs/$FSTYP/$dev
> +#
> +# Usage example:
> +#   _require_fs_sysfs_attr /dev/mapper/scratch-dev error/fail_at_unmount
> +_require_fs_sysfs_attr()
> +{
> +	_has_fs_sysfs_attr "$@" && return
> +
> +	local dev=$1
> +	local attr=$2
> +	local dname=$(_fs_sysfs_dname $dev)
> +
> +	_notrun "This test requires /sys/fs/${FSTYP}/${dname}/${attr}"
> +}
> +
> +# Test for the existence of a sysfs entry at /sys/fs/$FSTYP/DEV/$ATTR
> +#
> +# Only one argument is needed:
> +#  - attr: path name under /sys/fs/$FSTYP/DEV
> +#
> +# Usage example:
> +#   _has_fs_sysfs error/fail_at_unmount
> +_has_fs_sysfs()
> +{
> +	_has_fs_sysfs_attr $TEST_DEV "$@"
> +}
> +
> +# Require the existence of a sysfs entry at /sys/fs/$FSTYP/DEV/$ATTR
> +_require_fs_sysfs()
> +{
> +	_has_fs_sysfs "$@" && return
> +
> +	local attr=$1
> +	local dname=$(_short_dev $TEST_DEV)
> +
> +	_notrun "This test requires /sys/fs/${FSTYP}/${dname}/${attr}"
> +}
> +
>   # Generic test for specific filesystem feature.
>   # Currently only implemented to test overlayfs features.
>   _require_scratch_feature()