Re: [LTP] [PATCH v3 2/5] API/cgroup: Declare required controllers and version in test struct

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Li Wang <liwang@redhat.com> writes:

> Hi Richard,
>
> Richard Palethorpe <rpalethorpe@suse.com> wrote:
>  
>
>  --- a/include/tst_cgroup.h
>  +++ b/include/tst_cgroup.h
>  ...
>
>   /* A Control Group in LTP's aggregated hierarchy */
>   struct tst_cgroup_group;
>
>  +/* Populated with a reference to this tests's CGroup */
>  +extern const struct tst_cgroup_group *const tst_cgroup;
>  +extern const struct tst_cgroup_group *const tst_cgroup_drain;
>
>  
>
>  --- a/lib/tst_cgroup.c
>  +++ b/lib/tst_cgroup.c
>  @@ -18,7 +18,6 @@
>   #include "lapi/mount.h"
>   #include "lapi/mkdirat.h"
>   #include "tst_safe_file_at.h"
>  -#include "tst_cgroup.h"
>
>   struct cgroup_root;
>
>  @@ -138,6 +137,14 @@ struct tst_cgroup_group {
>          struct cgroup_dir *dirs[ROOTS_MAX + 1];
>   };
>
>  +/* If controllers are required via the tst_test struct then this is
>  + * populated with the test's CGroup.
>  + */
>  +static struct tst_cgroup_group test_group;
>  +static struct tst_cgroup_group drain_group;
>  +const struct tst_cgroup_group *const tst_cgroup = &test_group;
>
> What about declaring as 'tst_cgroup_test' ? i.e.
>
>     const struct tst_cgroup_group *const tst_cgroup_test = &test_group;
>
> As it is a reference to test's CGroup and the test_dir is the same
> layer with drain_dir, so this will be easier to understand the relationship
> with drain_group.
>
>  +const struct tst_cgroup_group *const tst_cgroup_drain =
>  &drain_group;

I agree with your logic, but the variable name is too long even without
'_test'. Perhaps we could shorten cgroup to cg? However I can submit a
separate patch for that.

>
> The rest part looks quite good.
> Reviewed-by: Li Wang <liwang@redhat.com>


-- 
Thank you,
Richard.

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