<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 17 Nov 2022 at 22:17, Kamil Konieczny <<a href="mailto:kamil.konieczny@linux.intel.com">kamil.konieczny@linux.intel.com</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 Craig,<br>
<br>
On 2022-11-17 at 10:41:29 +0200, Petri Latvala wrote:<br>> +#ifdef HAVE_LIBPROCPS<br>
> #include <proc/readproc.h><br>
> +#endif<br>
> +#ifdef HAVE_LIBPROC2<br>
-- ^<br>
Maybe just #elif ?<br></blockquote><div>That's probably better, you don't want both to happen.</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">-- ^^^^^^^^^^^^^^^^^^<br>
> +#include <dirent.h><br>
-- ^^^^^^^^^^^^^^^^^^<br>
What this has with libproc2 ?<br>
Note also that FreeBSD have its own limits.h<br></blockquote><div>I'm not sure why that would be there, the library handles the directory enumeration.</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">
<br>> + if (!strncasecmp(pid_comm, comm, strlen(pid_comm))) {<br>
<br>
Processes AA and aa are different, why do you use strncasecmp ?<br></blockquote><div>I agree, but the original uses strncasecmp. They both should use strncmp but that's beyond "making it work with libproc2".</div><div>Otherwise linking with libprocps is case-insenstive and linking with libproc2 is case sensitive. </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"><br>
> @@ -1524,10 +1616,13 @@ static void pipewire_reserve_wait(void)<br>
> char xdg_dir[PATH_MAX];<br>
> const char *homedir;<br>
> struct passwd *pw;<br>
> - proc_t *proc_info;<br>
> - PROCTAB *proc;<br>
> + int tid=0, euid, egid;<br>
> <br>
> +#ifdef HAVE_LIBPROCPS<br>
> igt_fork_helper(&pw_reserve_proc) {<br>
> + proc_t *proc_info;<br>
> + PROCTAB *proc;<br>
> +<br>
> igt_info("Preventing pipewire-pulse to use the audio drivers\n");<br>
> <br>
> proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);<br>
> @@ -1540,19 +1635,44 @@ static void pipewire_reserve_wait(void)<br>
> }<br>
> closeproc(proc);<br>
> <br>
> + tid = proc_info->tid;<br>
> + euid = proc_info->euid;<br>
> + egid = proc_info->egid;<br>
> + freeproc(proc_info);<br>
> +#endif<br>
> +#ifdef HAVE_LIBPROC2<br>
> + igt_fork(child, 1) {<br>
<br>
This should be fork_helper like above.<br></blockquote><div>Did something change here recently? My version of pipewire_reserve_wait() has igt_fork()</div><div>However, whatever the current version is doing they should be the same.</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">
<br>
> + enum pids_item Items[] = { PIDS_ID_PID, PIDS_ID_EUID, PIDS_ID_EGID };<br>
> + enum rel_items { EU_PID, EU_EUID, EU_EGID };<br>
> + struct pids_info *info = NULL;<br>
> + struct pids_stack *stack;<br>
> +<br>
> + igt_info("Preventing pipewire-pulse to use the audio drivers\n");<br>
> +<br>
> + if (procps_pids_new(&info, Items, 3) < 0)<br>
> + return;<br>
------------------- ^<br>
Use exit(0) here plus maybe some ing_info why it failed ?<br></blockquote><div>Ah yes, because at this point you're running as a child process. </div><div>The error message we (procps) use for this is:</div><div>"Unable to create pid info structure"<br></div><div>So this would be</div><div>{</div><div> igt_info("Unable to create pid info structure");</div><div> exit(0);</div><div>}</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> @@ -1584,6 +1702,10 @@ int pipewire_pulse_start_reserve(void)<br>
> * pipewire version 0.3.50 or upper.<br>
> */<br>
> for (attempts = 0; attempts < PIPEWIRE_RESERVE_MAX_TIME; attempts++) {<br>
> +#ifdef HAVE_LIBPROCPS<br>
> + PROCTAB *proc;<br>
> + proc_t *proc_info;<br>
> +<br>
> usleep(1000);<br>
> proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);<br>
> igt_assert(proc != NULL);<br>
> @@ -1598,6 +1720,25 @@ int pipewire_pulse_start_reserve(void)<br>
> freeproc(proc_info);<br>
> }<br>
> closeproc(proc);<br>
> +#endif<br>
> +#ifdef HAVE_LIBPROC2<br>
> + enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD };<br>
> + struct pids_info *info = NULL;<br>
> + struct pids_stack *stack;<br>
> +<br>
> + usleep(1000);<br>
> +<br>
> + if (procps_pids_new(&info, Items, 2) < 0)<br>
> + return 1;<br>
----------------------- ^<br>
break or igt_assert_f(errno == 0, "Getting procps failed\n"); here.<br></blockquote><div>If procps_pids_new() returns a negative number then getting the info structure has failed.</div><div>This is very similar to igt_assert(proc != NULL) above in the same function.</div><div>I'm not sure of your coding style, should you assert that procps_pids_new returns >= 0? or use the branch?</div><div><br></div><div>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.</div><div><br></div><div> - Craig</div><div><br></div></div></div>