Re: [PATCH v3 1/2] common/rc: add functions to check or write objects under /sys/fs/$FSTYP

[Eryu Guan <eguan@redhat.com>]

On Sun, May 29, 2016 at 12:28:40PM +0800, Zorro Lang wrote:
> XFS add more configurations in /sys/fs/xfs recently. For use
> them, this patch add some common functions for:
>   1. "require" a file/dir in /sys/fs/${FSTYP}.
>   2. write a file in /sys/fs/${FSTYP}.
> 
> For common use, these functions can be used by other filesystems.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
> 
> Hi,
> 
> V3 revert dname=$(basename $(readlink -f $dev)) line.
> 
> I think we don't need to check if /sys/fs/${FSTYP}/${dname}/$target
> is exist in _set_fs_sys_fs_param(). Because if no this file,
> *echo* will report error, and it will disturb golden file.
> 
> Thanks,
> Zorro
> 
>  common/rc | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 51092a0..759ec30 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3556,6 +3556,46 @@ run_fsx()
>  	fi
>  }
>  
> +_require_fs_sys_fs()

_require_fs_sysfs seems better to me :)

And a comment about the purpose of this helper would be fine, e.g. the
comment Eric wrote in his review:

"Test for the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ENTRY"

> +{
> +	local dev=$1
> +	local target=$2
> +	local tmp_mnt=`mktemp -d`
> +
> +	if [ ! -b "$dev" -o -z "$target" ];then
> +		_fail "Usage: _require_fs_sys_fs <device> <sysfs_path>"
> +        fi
> +
> +	local dname=$(basename $(readlink -f $dev))

We do have a helper "_short_dev" to do this, e.g.

	local dname=`_short_dev $dev`

> +	_mount -t $FSTYP `_common_dev_mount_options` $dev $tmp_mnt
> +	if [ $? -ne 0 ];then
> +		rm -f $tmp_mnt
> +		_fail "could not mount; mkfs first in your test?"

Having function name as prefix in error message makes debugging easier,
I think.

> +	elif [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> +		umount $tmp_mnt

Use $UMOUNT_PROG here, and in all other umount calls.

> +		rm -f $tmp_mnt
> +		_notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> +	fi
> +	umount $tmp_mnt
> +	rm -rf $tmp_mnt
> +}
> +
> +_set_fs_sys_fs_param()

_set_fs_sysfs_param :) And a comment about its purpose.

Thanks,
Eryu

> +{
> +	local dev=$1
> +	shift
> +	local target=$1
> +	shift
> +	local content="$*"
> +
> +	if [ ! -b "$dev" -o -z "$target" -o -z "$content" ];then
> +		_fail "Usage: _set_sys_fs_param <mounted_device> <target> <content>"
> +	fi
> +
> +	local dname=$(basename $(readlink -f $dev))
> +	echo "$content" > /sys/fs/${FSTYP}/${dname}/$target
> +}
> +
>  init_rc
>  
>  ################################################################################
> -- 
> 2.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html