Hi Petr,

On Tue, Jul 26, 2022 at 6:12 AM Petr Vorel <pvorel@suse.cz> wrote:

> 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")
>
>
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.


> > +     [ "$_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).
>
>
That's a good point. I'll just save the return and propagate the return
code.



> 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).
>
>
Yeah, I will rebase and resubmit with the fixes people have mentioned.


> Kind regards,
> Petr
>