<div dir="ltr"><div>Hi Petr, <br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 26, 2022 at 6:12 AM Petr Vorel <<a href="mailto:pvorel@suse.cz">pvorel@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Luke,<br>
<br>
...<br>
> +# Gets the cgroup version of the given controller<br>
> +# USAGE: cgroup_get_version CONTROLLER<br>
> +# RETURNS: "1" if version 1 and "2" if version 2<br>
> +# Must call cgroup_require before calling<br>
> +cgroup_get_version()<br>
>  {<br>
> -     local subsystem=$1<br>
> -     local mntpoint<br>
> +     local ctrl="$1"<br>
> +     local version<br>
<br>
> -     [ $# -eq 0 ] && tst_brk TBROK "get_cgroup_mountpoint: subsystem not defined"<br>
> +     [ $# -eq 0 ] && tst_brk TBROK "cgroup_get_version: controller not defined"<br>
NOTE: this will always pass, because you pass variable in ""<br>
(thus $1 = "" and $# = 1):<br>
cgroup_get_task_list()<br>
{<br>
        local ctrl="$1"<br>
        version=$(cgroup_get_version "$ctrl")<br>
<br></blockquote><div><br></div><div>Yes this is true when using this function by other functions in the library. The use case where this would fail is when someone calls it from a test and forgets to pass a controller to it or something like that.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +     [ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_version: No previous state found. Forgot to call cgroup_require?"<br>
<br>
> -     mntpoint=$(grep cgroup /proc/mounts | grep -w $subsystem | awk '{ print $2 }')<br>
> -     [ -z "$mntpoint" ] && return 1<br>
> +     version=$(echo "$_cgroup_state" | grep -w "^$ctrl" | awk '{ print $2 }')<br>
> +     [ "$version" = "" ] && tst_brk TBROK "cgroup_get_version: Could not find controller $ctrl"<br>
> +<br>
> +     echo "$version"<br>
<br>
> -     echo $mntpoint<br>
>       return 0<br>
>  }<br>
...<br>
<br>
> +# Mounts and configures the given controller<br>
> +# USAGE: cgroup_require CONTROLLER<br>
> +cgroup_require()<br>
> +{<br>
> +     local ctrl="$1"<br>
> +<br>
> +     [ $# -eq 0 ] && tst_brk TBROK "cgroup_require: controller not defined"<br>
> +<br>
> +     [ ! -f /proc/cgroups ] && tst_brk TCONF "Kernel does not support control groups"<br>
> +<br>
> +     _cgroup_state=$(tst_cgctl require "$ctrl" $$)<br>
> +<br>
> +     if [ $? -eq 32 ]; then<br>
> +             tst_brk TCONF "'tst_cgctl require' exited. Controller is probably not available?"<br>
> +     fi<br>
> +<br>
> +     if [ $? -ne 0 ]; then<br>
> +             tst_brk TBROK "'tst_cgctl require' exited."<br>
> +     fi<br>
FYI if cgroup_require is called from cleanup function tst_brk does not exit the<br>
code:<br>
<br>
tst_brk()<br>
{<br>
        local res=$1<br>
        shift<br>
<br>
        if [ "$TST_DO_EXIT" = 1 ]; then<br>
                tst_res TWARN "$@"<br>
                return<br>
        fi<br>
<br>
        tst_res "$res" "$@"<br>
        _tst_do_exit<br>
}<br>
<br>
IMHO that means that $? became 0 even it was previously 32.<br>
It's always safer to save $? into local variable if needed to store exit code<br>
(otherwise using if, e.g. "if ! foo; then" is preferred).<br>
<br></blockquote><div><br></div><div>That's a good point. I'll just save the return and propagate the return code.<br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
NOTE: Maybe at this point it might be safer if you post next version<br>
where you do fixes yourself. I'll try to review the rest of the shell scripts<br>
today (C code looks correct to me).<br>
<br></blockquote><div><br></div><div>Yeah, I will rebase and resubmit with the fixes people have mentioned.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Kind regards,<br>
Petr<br>
</blockquote></div></div>