[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Jan Stancek <jstancek@redhat.com>]

----- Original Message -----
> On Fri, Jun 5, 2020 at 6:14 PM Jan Stancek <jstancek@redhat.com> wrote:
> 
> > On Wed, Jun 03, 2020 at 08:51:37PM +0800, Li Wang wrote:
> > >> I don't get why global variables are necessary.
> > >>
> > >
> > >The only reason to export them as global variables is to make the legacy
> > >read/write_cpuse_files() happy. So that I said it is a compromise.
> > >
> > >$ git grep tst_cgroup_new_path
> > >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "cpus",
> > >buf);
> > >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
> > >mems);
> > >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
> > >buf);
> > >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
> > >buf);
> > >lib/mem.c:      write_cpuset_files(tst_cgroup_new_path, "mems", buf);
> > >lib/mem.c:              write_cpuset_files(tst_cgroup_new_path, "cpus",
> > >cpus);
> > >lib/mem.c:              write_cpuset_files(tst_cgroup_new_path, "cpus",
> > >"0");
> > >oom/oom04.c:            write_cpuset_files(tst_cgroup_new_path,
> > >"memory_migrate", "1");
> > >oom/oom05.c:            write_cpuset_files(tst_cgroup_new_path,
> > >"memory_migrate", "1");
> >
> > What if we provided access to it via API? Would we still need these
> > global variables?
> >
> >   char *tst_cgroup_get_path(const char *cgroup_mnt)
> >       // return ptr to tst_cgroup_paths->new_path
> >
> 
> The series of list operating function are hiding in the library. My thought
> is
> to make the list transparent to users.
> 
> In your method, we have to export the tst_cgroup_get_path() as an external
> function, it stills needs an extra local pointer in testcase to store the
> got new_path,
> it doesn't seem tidier too.

But there would be clear connection between function and variable.
  new_path = tst_cgroup_get_path(cgroup_dir);
vs.
  tst_cgroup_get_path(cgroup_dir);
  // fyi, tst_cgroup_new_path is updated as side-effect of call above
  // What other calls do update tst_cgroup_new_path? Have a look at implementation.

> 
> 
> > mount path is always known to test, because it passes it to
> > tst_cgroup_mount(),
> > so it just needs to find out "new path".
> >
> > Would that satisfy the need of this legacy test?
> 
> 
> How about moving the cpuset legacy code to the library as part of APIs?
> That'd
> help to capsulate all the operation of cgroup control in lib, and people
> just need
> to invoke the related function as what he/she wants.
> 
> +void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char
> *filename, char *buf);
> +void tst_cgroup_cpuset_write_files(const char *cgroup_dir, const char
> *filename, const char *buf);
> 
> Then 'tst_cgroup_new_path' searching work will all located internally. And
> 'tst_cgroup_ctl_knob' can
> be local variable too.
> 
> Any other comments? (attach the v3.1)

That makes it somewhat better, since it's only concern of library code now.
But since there are no tests using "tst_cgroup_new_path", does it still
need to be global variable?