Re: [LTP] [PATCH v5 10/18] controllers: Expand cgroup_lib shell library

[Petr Vorel <pvorel@suse.cz>]

Hi Luke,

...
> +# Gets the cgroup version of the given controller
> +# USAGE: cgroup_get_version CONTROLLER
> +# RETURNS: "1" if version 1 and "2" if version 2
> +# Must call cgroup_require before calling
> +cgroup_get_version()
>  {
> -	local subsystem=$1
> -	local mntpoint
> +	local ctrl="$1"
> +	local version

> -	[ $# -eq 0 ] && tst_brk TBROK "get_cgroup_mountpoint: subsystem not defined"
> +	[ $# -eq 0 ] && tst_brk TBROK "cgroup_get_version: controller not defined"
NOTE: this will always pass, because you pass variable in ""
(thus $1 = "" and $# = 1):
cgroup_get_task_list()
{
	local ctrl="$1"
	version=$(cgroup_get_version "$ctrl")

> +	[ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_version: No previous state found. Forgot to call cgroup_require?"

> -	mntpoint=$(grep cgroup /proc/mounts | grep -w $subsystem | awk '{ print $2 }')
> -	[ -z "$mntpoint" ] && return 1
> +	version=$(echo "$_cgroup_state" | grep -w "^$ctrl" | awk '{ print $2 }')
> +	[ "$version" = "" ] && tst_brk TBROK "cgroup_get_version: Could not find controller $ctrl"
> +
> +	echo "$version"

> -	echo $mntpoint
>  	return 0
>  }
...

> +# Mounts and configures the given controller
> +# USAGE: cgroup_require CONTROLLER
> +cgroup_require()
> +{
> +	local ctrl="$1"
> +
> +	[ $# -eq 0 ] && tst_brk TBROK "cgroup_require: controller not defined"
> +
> +	[ ! -f /proc/cgroups ] && tst_brk TCONF "Kernel does not support control groups"
> +
> +	_cgroup_state=$(tst_cgctl require "$ctrl" $$)
> +
> +	if [ $? -eq 32 ]; then
> +		tst_brk TCONF "'tst_cgctl require' exited. Controller is probably not available?"
> +	fi
> +
> +	if [ $? -ne 0 ]; then
> +		tst_brk TBROK "'tst_cgctl require' exited."
> +	fi
FYI if cgroup_require is called from cleanup function tst_brk does not exit the
code:

tst_brk()
{
	local res=$1
	shift

	if [ "$TST_DO_EXIT" = 1 ]; then
		tst_res TWARN "$@"
		return
	fi

	tst_res "$res" "$@"
	_tst_do_exit
}

IMHO that means that $? became 0 even it was previously 32.
It's always safer to save $? into local variable if needed to store exit code
(otherwise using if, e.g. "if ! foo; then" is preferred).

NOTE: Maybe at this point it might be safer if you post next version
where you do fixes yourself. I'll try to review the rest of the shell scripts
today (C code looks correct to me).

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp