[igt-dev] [PATCH i-g-t] Use the new procps library libproc2

[Craig Small]

On Thu, 17 Nov 2022 at 22:17, Kamil Konieczny <
kamil.konieczny at linux.intel.com> wrote:

> Hi Craig,
>
> On 2022-11-17 at 10:41:29 +0200, Petri Latvala wrote:
> > +#ifdef HAVE_LIBPROCPS
> >  #include <proc/readproc.h>
> > +#endif
> > +#ifdef HAVE_LIBPROC2
> -- ^
> Maybe just #elif ?
>
That's probably better, you don't want both to happen.


> -- ^^^^^^^^^^^^^^^^^^
> > +#include <dirent.h>
> -- ^^^^^^^^^^^^^^^^^^
> What this has with libproc2 ?
> Note also that FreeBSD have its own limits.h
>
I'm not sure why that would be there, the library handles the directory
enumeration.


>
> > +         if (!strncasecmp(pid_comm, comm, strlen(pid_comm))) {
>
> Processes AA and aa are different, why do you use strncasecmp ?
>
I agree, but the original uses strncasecmp. They both should use strncmp
but that's beyond "making it work with libproc2".
Otherwise linking with libprocps is case-insenstive and linking with
libproc2 is case sensitive.


>
> > @@ -1524,10 +1616,13 @@ static void pipewire_reserve_wait(void)
> >       char xdg_dir[PATH_MAX];
> >       const char *homedir;
> >       struct passwd *pw;
> > -     proc_t *proc_info;
> > -     PROCTAB *proc;
> > +     int tid=0, euid, egid;
> >
> > +#ifdef HAVE_LIBPROCPS
> >       igt_fork_helper(&pw_reserve_proc) {
> > +             proc_t *proc_info;
> > +             PROCTAB *proc;
> > +
> >               igt_info("Preventing pipewire-pulse to use the audio
> drivers\n");
> >
> >               proc = openproc(PROC_FILLCOM | PROC_FILLSTAT |
> PROC_FILLARG);
> > @@ -1540,19 +1635,44 @@ static void pipewire_reserve_wait(void)
> >               }
> >               closeproc(proc);
> >
> > +             tid = proc_info->tid;
> > +             euid = proc_info->euid;
> > +             egid = proc_info->egid;
> > +             freeproc(proc_info);
> > +#endif
> > +#ifdef HAVE_LIBPROC2
> > +     igt_fork(child, 1) {
>
> This should be fork_helper like above.
>
Did something change here recently? My version of pipewire_reserve_wait()
has igt_fork()
However, whatever the current version is doing they should be the same.


>
> > +             enum pids_item Items[] = { PIDS_ID_PID, PIDS_ID_EUID,
> PIDS_ID_EGID };
> > +             enum rel_items { EU_PID, EU_EUID, EU_EGID };
> > +             struct pids_info *info = NULL;
> > +             struct pids_stack *stack;
> > +
> > +             igt_info("Preventing pipewire-pulse to use the audio
> drivers\n");
> > +
> > +             if (procps_pids_new(&info, Items, 3) < 0)
> > +                 return;
> ------------------- ^
> Use exit(0) here plus maybe some ing_info why it failed ?
>
Ah yes, because at this point you're running as a child process.
The error message we (procps) use for this is:
"Unable to create pid info structure"
So this would be
{
  igt_info("Unable to create pid info structure");
  exit(0);
}

> @@ -1584,6 +1702,10 @@ int pipewire_pulse_start_reserve(void)
> >        * pipewire version 0.3.50 or upper.
> >        */
> >       for (attempts = 0; attempts < PIPEWIRE_RESERVE_MAX_TIME;
> attempts++) {
> > +#ifdef HAVE_LIBPROCPS
> > +             PROCTAB *proc;
> > +             proc_t *proc_info;
> > +
> >               usleep(1000);
> >               proc = openproc(PROC_FILLCOM | PROC_FILLSTAT |
> PROC_FILLARG);
> >               igt_assert(proc != NULL);
> > @@ -1598,6 +1720,25 @@ int pipewire_pulse_start_reserve(void)
> >                       freeproc(proc_info);
> >               }
> >               closeproc(proc);
> > +#endif
> > +#ifdef HAVE_LIBPROC2
> > +             enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD };
> > +             struct pids_info *info = NULL;
> > +             struct pids_stack *stack;
> > +
> > +             usleep(1000);
> > +
> > +             if (procps_pids_new(&info, Items, 2) < 0)
> > +                     return 1;
> ----------------------- ^
> break or igt_assert_f(errno == 0, "Getting procps failed\n"); here.
>
If procps_pids_new() returns a negative number then getting the info
structure has failed.
This is very similar to igt_assert(proc != NULL) above in the same function.
I'm not sure of your coding style, should you assert that procps_pids_new
returns >= 0? or use the branch?

Thankyou for your review on this patch. The sole goal of this patch is so
igt does the same thing with libprocps and libproc2 so I've tried to keep
the same behavior.

  - Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20221118/2450feac/attachment.htm>