[igt-dev] [PATCH i-g-t 0/1] Use new library libproc2

[igt-dev] [PATCH i-g-t 0/1] Use new library libproc2

[Kamil Konieczny]

This is resend of https://patchwork.freedesktop.org/series/111015/
"Use the new procps library libproc2"

with some minor editions and rebase. With this igt will compile
on current Ubuntu 23.04

Cc: Craig Small <csmall@dropbear.xyz>
Cc: Petri Latvala <adrinael@adrinael.net>
Craig Small (1):
  Use the new procps library libproc2

 lib/igt_aux.c   | 243 ++++++++++++++++++++++++++++++++++++++++--------
 lib/meson.build |   7 +-
 meson.build     |  10 +-
 3 files changed, 218 insertions(+), 42 deletions(-)

-- 
2.37.2

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

[Kamil Konieczny]

From: Craig Small <csmall@dropbear.xyz>

Added support for new libproc2.

[Corrected some errors pointed by checkpatch.pl,
 add linux includes in #ifdef __linux__ section,
 use #elif HAVE_LIBPROC2 for new libproc2 code - Kamil]
Signed-off-by: Craig Small <csmall@dropbear.xyz>
---
 lib/igt_aux.c   | 243 ++++++++++++++++++++++++++++++++++++++++--------
 lib/meson.build |   7 +-
 meson.build     |  10 +-
 3 files changed, 218 insertions(+), 42 deletions(-)

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 672d7d4b0..db6c4da93 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -52,8 +52,19 @@
 #include <assert.h>
 #include <grp.h>
 
+#ifdef HAVE_LIBPROCPS
 #include <proc/readproc.h>
+#elif HAVE_LIBPROC2
+#include <libproc2/pids.h>
+#endif
+
+#include <dirent.h>
+#ifdef __linux__
 #include <libudev.h>
+#include <linux/limits.h>
+#else
+#include <limits.h>
+#endif
 
 #include "drmtest.h"
 #include "i915_drm.h"
@@ -1217,6 +1228,7 @@ void igt_unlock_mem(void)
  */
 int igt_is_process_running(const char *comm)
 {
+#if HAVE_LIBPROCPS
 	PROCTAB *proc;
 	proc_t *proc_info;
 	bool found = false;
@@ -1235,6 +1247,26 @@ int igt_is_process_running(const char *comm)
 
 	closeproc(proc);
 	return found;
+#elif HAVE_LIBPROC2
+	enum pids_item Item[] = { PIDS_CMD };
+	struct pids_info *info = NULL;
+	struct pids_stack *stack;
+	char *pid_comm;
+	bool found = false;
+
+	if (procps_pids_new(&info, Item, 1) < 0)
+		return false;
+	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
+		pid_comm = PIDS_VAL(0, str, stack, info);
+		if (!strncasecmp(pid_comm, comm, strlen(pid_comm))) {
+			found = true;
+			break;
+		}
+	}
+
+	procps_pids_unref(&info);
+	return found;
+#endif
 }
 
 /**
@@ -1251,6 +1283,7 @@ int igt_is_process_running(const char *comm)
  */
 int igt_terminate_process(int sig, const char *comm)
 {
+#if HAVE_LIBPROCPS
 	PROCTAB *proc;
 	proc_t *proc_info;
 	int err = 0;
@@ -1272,6 +1305,28 @@ int igt_terminate_process(int sig, const char *comm)
 
 	closeproc(proc);
 	return err;
+#elif HAVE_LIBPROC2
+	enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD };
+	struct pids_info *info = NULL;
+	struct pids_stack *stack;
+	char *pid_comm;
+	int pid;
+	int err = 0;
+
+	if (procps_pids_new(&info, Items, 2) < 0)
+		return -errno;
+	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
+		pid = PIDS_VAL(0, s_int, stack, info);
+		pid_comm = PIDS_VAL(1, str, stack, info);
+		if (!strncasecmp(pid_comm, comm, strlen(pid_comm))) {
+			if (kill(pid, sig) < 0)
+				err = -errno;
+			break;
+		}
+	}
+	procps_pids_unref(&info);
+	return err;
+#endif
 }
 
 struct pinfo {
@@ -1341,9 +1396,9 @@ igt_show_stat_header(void)
 }
 
 static void
-igt_show_stat(proc_t *info, int *state, const char *fn)
+igt_show_stat(const pid_t tid, const char *cmd, int *state, const char *fn)
 {
-	struct pinfo p = { .pid = info->tid, .comm = info->cmd, .fn = fn };
+	struct pinfo p = { .pid = tid, .comm = cmd, .fn = fn };
 
 	if (!*state)
 		igt_show_stat_header();
@@ -1353,7 +1408,7 @@ igt_show_stat(proc_t *info, int *state, const char *fn)
 }
 
 static void
-__igt_lsof_fds(proc_t *proc_info, int *state, char *proc_path, const char *dir)
+__igt_lsof_fds(const pid_t tid, const char *cmd, int *state, char *proc_path, const char *dir)
 {
 	struct dirent *d;
 	struct stat st;
@@ -1400,7 +1455,7 @@ again:
 		dirn = dirname(copy_fd_lnk);
 
 		if (!strncmp(dir, dirn, strlen(dir)))
-			igt_show_stat(proc_info, state, fd_lnk);
+			igt_show_stat(tid, cmd, state, fd_lnk);
 
 		free(copy_fd_lnk);
 		free(fd_lnk);
@@ -1416,13 +1471,13 @@ again:
 static void
 __igt_lsof(const char *dir)
 {
-	PROCTAB *proc;
-	proc_t *proc_info;
-
 	char path[30];
 	char *name_lnk;
 	struct stat st;
 	int state = 0;
+#ifdef HAVE_LIBPROCPS
+	PROCTAB *proc;
+	proc_t *proc_info;
 
 	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
 	igt_assert(proc != NULL);
@@ -1443,19 +1498,56 @@ __igt_lsof(const char *dir)
 		name_lnk[read] = '\0';
 
 		if (!strncmp(dir, name_lnk, strlen(dir)))
-			igt_show_stat(proc_info, &state, name_lnk);
+			igt_show_stat(proc_info->tid, proc_info->cmd, &state, name_lnk);
 
 		/* check also fd, seems that lsof(8) doesn't look here */
 		memset(path, 0, sizeof(path));
 		snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid);
 
-		__igt_lsof_fds(proc_info, &state, path, dir);
+		__igt_lsof_fds(proc_info->tid, proc_info->cmd, &state, path, dir);
 
 		free(name_lnk);
 		freeproc(proc_info);
 	}
 
 	closeproc(proc);
+#elif HAVE_LIBPROC2
+	enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD };
+	struct pids_info *info = NULL;
+	struct pids_stack *stack;
+
+	if (procps_pids_new(&info, Items, 2) < 0)
+		return;
+	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
+		ssize_t read;
+		int tid = PIDS_VAL(0, s_int, stack, info);
+		char *pid_comm = PIDS_VAL(1, str, stack, info);
+
+		/* check current working directory */
+		memset(path, 0, sizeof(path));
+		snprintf(path, sizeof(path), "/proc/%d/cwd", tid);
+
+		if (stat(path, &st) == -1)
+			continue;
+
+		name_lnk = malloc(st.st_size + 1);
+
+		igt_assert((read = readlink(path, name_lnk, st.st_size + 1)));
+		name_lnk[read] = '\0';
+
+		if (!strncmp(dir, name_lnk, strlen(dir)))
+			igt_show_stat(tid, pid_comm, &state, name_lnk);
+
+		/* check also fd, seems that lsof(8) doesn't look here */
+		memset(path, 0, sizeof(path));
+		snprintf(path, sizeof(path), "/proc/%d/fd", tid);
+
+		__igt_lsof_fds(tid, pid_comm, &state, path, dir);
+
+		free(name_lnk);
+	}
+	procps_pids_unref(&info);
+#endif
 }
 
 /**
@@ -1490,7 +1582,7 @@ igt_lsof(const char *dpath)
 	free(sanitized);
 }
 
-static void pulseaudio_unload_module(proc_t *proc_info)
+static void pulseaudio_unload_module(const uid_t euid, const gid_t egid)
 {
 	struct igt_helper_process pa_proc = {};
 	char xdg_dir[PATH_MAX];
@@ -1498,14 +1590,14 @@ static void pulseaudio_unload_module(proc_t *proc_info)
 	struct passwd *pw;
 
 	igt_fork_helper(&pa_proc) {
-		pw = getpwuid(proc_info->euid);
+		pw = getpwuid(euid);
 		homedir = pw->pw_dir;
-		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
+		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", euid);
 
 		igt_info("Request pulseaudio to stop using audio device\n");
 
-		setgid(proc_info->egid);
-		setuid(proc_info->euid);
+		setgid(egid);
+		setuid(euid);
 		clearenv();
 		setenv("HOME", homedir, 1);
 		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
@@ -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,43 @@ static void pipewire_reserve_wait(void)
 		}
 		closeproc(proc);
 
+		tid = proc_info->tid;
+		euid = proc_info->euid;
+		egid = proc_info->egid;
+		freeproc(proc_info);
+#elif HAVE_LIBPROC2
+	igt_fork(child, 1) {
+		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;
+		while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
+			tid = PIDS_VAL(EU_PID, s_int, stack, info);
+			if (pipewire_pulse_pid == tid)
+				break;
+		}
+		euid = PIDS_VAL(EU_EUID, s_int, stack, info);
+		egid = PIDS_VAL(EU_EGID, s_int, stack, info);
+		procps_pids_unref(&info);
+#endif
+
 		/* Sanity check: if it can't find the process, it means it has gone */
-		if (pipewire_pulse_pid != proc_info->tid)
+		if (pipewire_pulse_pid != tid)
 			exit(0);
 
-		pw = getpwuid(proc_info->euid);
+		pw = getpwuid(euid);
 		homedir = pw->pw_dir;
-		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
-		setgid(proc_info->egid);
-		setuid(proc_info->euid);
+		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", euid);
+		setgid(egid);
+		setuid(euid);
 		clearenv();
 		setenv("HOME", homedir, 1);
 		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
-		freeproc(proc_info);
 
 		/*
 		 * pw-reserve will run in background. It will only exit when
@@ -1570,9 +1689,7 @@ static void pipewire_reserve_wait(void)
 int pipewire_pulse_start_reserve(void)
 {
 	bool is_pw_reserve_running = false;
-	proc_t *proc_info;
 	int attempts = 0;
-	PROCTAB *proc;
 
 	if (!pipewire_pulse_pid)
 		return 0;
@@ -1584,6 +1701,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 +1719,24 @@ int pipewire_pulse_start_reserve(void)
 			freeproc(proc_info);
 		}
 		closeproc(proc);
+#elif 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;
+		while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
+			if (!strcmp(PIDS_VAL(1, str, stack, info), "pw-reserve")) {
+				is_pw_reserve_running = true;
+				pipewire_pw_reserve_pid = PIDS_VAL(0, s_int, stack, info);
+				break;
+			}
+		}
+		procps_pids_unref(&info);
+#endif
 		if (is_pw_reserve_running)
 			break;
 	}
@@ -1645,7 +1784,7 @@ void pipewire_pulse_stop_reserve(void)
  * If the check fails, it means that the process can simply be killed.
  */
 static int
-__igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
+__igt_lsof_audio_and_kill_proc(const pid_t tid, const char *cmd, const uid_t euid, const gid_t egid, char *proc_path)
 {
 	const char *audio_dev = "/dev/snd/";
 	char path[PATH_MAX * 2];
@@ -1670,10 +1809,10 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
 	 * 2) unload/unbind the the audio driver(s);
 	 * 3) stop the pw-reserve thread.
 	 */
-	if (!strcmp(proc_info->cmd, "pipewire-pulse")) {
+	if (!strcmp(cmd, "pipewire-pulse")) {
 		igt_info("process %d (%s) is using audio device. Should be requested to stop using them.\n",
-			 proc_info->tid, proc_info->cmd);
-		pipewire_pulse_pid = proc_info->tid;
+			 tid, cmd);
+		pipewire_pulse_pid = tid;
 		return 0;
 	}
 	/*
@@ -1685,9 +1824,9 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
 	 * will respawn them. So, just ignore here, they'll honor pw-reserve,
 	 * when the time comes.
 	 */
-	if (!strcmp(proc_info->cmd, "pipewire-media-session"))
+	if (!strcmp(cmd, "pipewire-media-session"))
 		return 0;
-	if (!strcmp(proc_info->cmd, "wireplumber"))
+	if (!strcmp(cmd, "wireplumber"))
 		return 0;
 
 	dp = opendir(proc_path);
@@ -1723,22 +1862,22 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
 		 * enough to unbind audio modules and won't cause race issues
 		 * with systemd trying to reload it.
 		 */
-		if (!strcmp(proc_info->cmd, "pulseaudio")) {
-			pulseaudio_unload_module(proc_info);
+		if (!strcmp(cmd, "pulseaudio")) {
+			pulseaudio_unload_module(euid, egid);
 			break;
 		}
 
 		/* For all other processes, just kill them */
 		igt_info("process %d (%s) is using audio device. Should be terminated.\n",
-				proc_info->tid, proc_info->cmd);
+				tid, cmd);
 
-		if (kill(proc_info->tid, SIGTERM) < 0) {
+		if (kill(tid, SIGTERM) < 0) {
 			igt_info("Fail to terminate %s (pid: %d) with SIGTERM\n",
-				proc_info->cmd, proc_info->tid);
-			if (kill(proc_info->tid, SIGABRT) < 0) {
+				cmd, tid);
+			if (kill(tid, SIGABRT) < 0) {
 				fail++;
 				igt_info("Fail to terminate %s (pid: %d) with SIGABRT\n",
-					proc_info->cmd, proc_info->tid);
+					cmd, tid);
 			}
 		}
 
@@ -1760,23 +1899,47 @@ int
 igt_lsof_kill_audio_processes(void)
 {
 	char path[PATH_MAX];
+	int fail = 0;
+
+#ifdef HAVE_LIBPROCPS
 	proc_t *proc_info;
 	PROCTAB *proc;
-	int fail = 0;
 
 	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
 	igt_assert(proc != NULL);
 	pipewire_pulse_pid = 0;
-
 	while ((proc_info = readproc(proc, NULL))) {
 		if (snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid) < 1)
 			fail++;
 		else
-			fail += __igt_lsof_audio_and_kill_proc(proc_info, path);
+			fail += __igt_lsof_audio_and_kill_proc(proc_info->tid, proc_info->cmd, proc_info->euid, proc_info->egid, path);
 
 		freeproc(proc_info);
 	}
 	closeproc(proc);
+#elif HAVE_LIBPROC2
+	enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD, PIDS_ID_EUID, PIDS_ID_EGID };
+	enum rel_items { EU_PID, EU_CMD, EU_EUID, EU_EGID };
+	struct pids_info *info = NULL;
+	struct pids_stack *stack;
+	pid_t tid;
+
+	if (procps_pids_new(&info, Items, 4) < 0)
+		return 1;
+	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
+		tid = PIDS_VAL(EU_PID, s_int, stack, info);
+
+		if (snprintf(path, sizeof(path), "/proc/%d/fd", tid) < 1)
+			fail++;
+		else
+			fail += __igt_lsof_audio_and_kill_proc(tid,
+				PIDS_VAL(EU_CMD, str, stack, info),
+				PIDS_VAL(EU_EUID, s_int, stack, info),
+				PIDS_VAL(EU_EGID, s_int, stack, info),
+				path);
+	}
+	procps_pids_unref(&info);
+#endif
 
 	return fail;
 }
diff --git a/lib/meson.build b/lib/meson.build
index b21c252b5..48a27a612 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -113,7 +113,6 @@ lib_deps = [
 	libdrm,
 	libdw,
 	libkmod,
-	libprocps,
 	libudev,
 	math,
 	pciaccess,
@@ -177,6 +176,12 @@ if chamelium.found()
 	lib_sources += 'monitor_edids/monitor_edids_helper.c'
 endif
 
+if libprocps.found()
+	lib_deps += libprocps
+else
+	lib_deps += libproc2
+endif
+
 if get_option('srcdir') != ''
     srcdir = join_paths(get_option('srcdir'), 'tests')
 else
diff --git a/meson.build b/meson.build
index 036217844..98acc8daf 100644
--- a/meson.build
+++ b/meson.build
@@ -124,7 +124,15 @@ build_info += 'With libdrm: ' + ','.join(libdrm_info)
 
 pciaccess = dependency('pciaccess', version : '>=0.10')
 libkmod = dependency('libkmod')
-libprocps = dependency('libprocps', required : true)
+libprocps = dependency('libprocps', required : false)
+libproc2 = dependency('libproc2', required : false)
+if libprocps.found()
+  config.set('HAVE_LIBPROCPS', 1)
+elif libproc2.found()
+  config.set('HAVE_LIBPROC2', 1)
+else
+  error('Either libprocps or libproc2 is required')
+endif
 
 libunwind = dependency('libunwind', required : get_option('libunwind'))
 build_info += 'With libunwind: @0@'.format(libunwind.found())
-- 
2.37.2

[igt-dev] ✗ Fi.CI.BAT: failure for Use new library libproc2

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4191 bytes --]

== Series Details ==

Series: Use new library libproc2
URL   : https://patchwork.freedesktop.org/series/116896/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13054 -> IGTPW_8848
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_8848 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_8848, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/index.html

Participating hosts (38 -> 37)
------------------------------

  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_8848:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_store@basic:
    - fi-rkl-11600:       [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13054/fi-rkl-11600/igt@gem_exec_store@basic.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/fi-rkl-11600/igt@gem_exec_store@basic.html

  
Known issues
------------

  Here are the changes found in IGTPW_8848 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - bat-rpls-1:         NOTRUN -> [ABORT][3] ([i915#6687] / [i915#7978])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_selftest@live@slpc:
    - bat-rpls-1:         NOTRUN -> [DMESG-FAIL][4] ([i915#6367])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-rpls-1/igt@i915_selftest@live@slpc.html

  * igt@kms_pipe_crc_basic@read-crc:
    - bat-adlp-9:         NOTRUN -> [SKIP][5] ([i915#3546]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-adlp-9/igt@kms_pipe_crc_basic@read-crc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@migrate:
    - bat-adlp-6:         [DMESG-FAIL][6] ([i915#7699]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13054/bat-adlp-6/igt@i915_selftest@live@migrate.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-adlp-6/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@requests:
    - bat-rpls-1:         [ABORT][8] ([i915#4983] / [i915#7911]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13054/bat-rpls-1/igt@i915_selftest@live@requests.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-rpls-1/igt@i915_selftest@live@requests.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1:
    - bat-dg2-8:          [FAIL][10] ([i915#7932]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13054/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html

  
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7266 -> IGTPW_8848

  CI-20190529: 20190529
  CI_DRM_13054: 8cabe2adb8e028197f9535daffd3d5ff98d51d8b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8848: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/index.html
  IGT_7266: 94411dd85f9ad6d76fb7b2097197122703a3914c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/index.html

[-- Attachment #2: Type: text/html, Size: 4965 bytes --]

Re: [igt-dev] ✗ Fi.CI.BAT: failure for Use new library libproc2

[Kamil Konieczny]

Hi Sai,

this is unrelated to change (on older sys these will use
the same old libproc in igt_core).

Regards,
Kamil

On 2023-04-24 at 17:13:25 -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: Use new library libproc2
> URL   : https://patchwork.freedesktop.org/series/116896/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_13054 -> IGTPW_8848
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with IGTPW_8848 absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in IGTPW_8848, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/index.html
> 
> Participating hosts (38 -> 37)
> ------------------------------
> 
>   Missing    (1): fi-snb-2520m 
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in IGTPW_8848:
> 
> ### IGT changes ###
> 
> #### Possible regressions ####
> 
>   * igt@gem_exec_store@basic:
>     - fi-rkl-11600:       [PASS][1] -> [ABORT][2]
>    [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13054/fi-rkl-11600/igt@gem_exec_store@basic.html
>    [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/fi-rkl-11600/igt@gem_exec_store@basic.html
> 
>   
> Known issues
> ------------
> 
>   Here are the changes found in IGTPW_8848 that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@gem_exec_suspend@basic-s3@smem:
>     - bat-rpls-1:         NOTRUN -> [ABORT][3] ([i915#6687] / [i915#7978])
>    [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html
> 
>   * igt@i915_selftest@live@slpc:
>     - bat-rpls-1:         NOTRUN -> [DMESG-FAIL][4] ([i915#6367])
>    [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-rpls-1/igt@i915_selftest@live@slpc.html
> 
>   * igt@kms_pipe_crc_basic@read-crc:
>     - bat-adlp-9:         NOTRUN -> [SKIP][5] ([i915#3546]) +1 similar issue
>    [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-adlp-9/igt@kms_pipe_crc_basic@read-crc.html
> 
>   
> #### Possible fixes ####
> 
>   * igt@i915_selftest@live@migrate:
>     - bat-adlp-6:         [DMESG-FAIL][6] ([i915#7699]) -> [PASS][7]
>    [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13054/bat-adlp-6/igt@i915_selftest@live@migrate.html
>    [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-adlp-6/igt@i915_selftest@live@migrate.html
> 
>   * igt@i915_selftest@live@requests:
>     - bat-rpls-1:         [ABORT][8] ([i915#4983] / [i915#7911]) -> [PASS][9]
>    [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13054/bat-rpls-1/igt@i915_selftest@live@requests.html
>    [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-rpls-1/igt@i915_selftest@live@requests.html
> 
>   * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1:
>     - bat-dg2-8:          [FAIL][10] ([i915#7932]) -> [PASS][11]
>    [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13054/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html
>    [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html
> 
>   
>   [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
>   [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
>   [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
>   [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
>   [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
>   [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
>   [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
>   [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978
> 
> 
> Build changes
> -------------
> 
>   * CI: CI-20190529 -> None
>   * IGT: IGT_7266 -> IGTPW_8848
> 
>   CI-20190529: 20190529
>   CI_DRM_13054: 8cabe2adb8e028197f9535daffd3d5ff98d51d8b @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGTPW_8848: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/index.html
>   IGT_7266: 94411dd85f9ad6d76fb7b2097197122703a3914c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8848/index.html

Re: [igt-dev] [PATCH i-g-t 1/1] Use the new procps library libproc2

[Kamil Konieczny]

Hi,

I forgot that there is one place which possibly
need a rework, see below.

On 2023-04-24 at 18:26:42 +0200, Kamil Konieczny wrote:
> From: Craig Small <csmall@dropbear.xyz>
> 
> Added support for new libproc2.
> 
> [Corrected some errors pointed by checkpatch.pl,
>  add linux includes in #ifdef __linux__ section,
>  use #elif HAVE_LIBPROC2 for new libproc2 code - Kamil]
> Signed-off-by: Craig Small <csmall@dropbear.xyz>
> ---
>  lib/igt_aux.c   | 243 ++++++++++++++++++++++++++++++++++++++++--------
>  lib/meson.build |   7 +-
>  meson.build     |  10 +-
>  3 files changed, 218 insertions(+), 42 deletions(-)
> 
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index 672d7d4b0..db6c4da93 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -52,8 +52,19 @@
>  #include <assert.h>
>  #include <grp.h>
>  
> +#ifdef HAVE_LIBPROCPS
>  #include <proc/readproc.h>
> +#elif HAVE_LIBPROC2
> +#include <libproc2/pids.h>
> +#endif
> +
> +#include <dirent.h>
> +#ifdef __linux__
>  #include <libudev.h>
> +#include <linux/limits.h>
> +#else
> +#include <limits.h>
> +#endif
>  
>  #include "drmtest.h"
>  #include "i915_drm.h"
> @@ -1217,6 +1228,7 @@ void igt_unlock_mem(void)
>   */
>  int igt_is_process_running(const char *comm)
>  {
> +#if HAVE_LIBPROCPS
>  	PROCTAB *proc;
>  	proc_t *proc_info;
>  	bool found = false;
> @@ -1235,6 +1247,26 @@ int igt_is_process_running(const char *comm)
>  
>  	closeproc(proc);
>  	return found;
> +#elif HAVE_LIBPROC2
> +	enum pids_item Item[] = { PIDS_CMD };
> +	struct pids_info *info = NULL;
> +	struct pids_stack *stack;
> +	char *pid_comm;
> +	bool found = false;
> +
> +	if (procps_pids_new(&info, Item, 1) < 0)
> +		return false;
> +	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> +		pid_comm = PIDS_VAL(0, str, stack, info);
> +		if (!strncasecmp(pid_comm, comm, strlen(pid_comm))) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	procps_pids_unref(&info);
> +	return found;
> +#endif
>  }
>  
>  /**
> @@ -1251,6 +1283,7 @@ int igt_is_process_running(const char *comm)
>   */
>  int igt_terminate_process(int sig, const char *comm)
>  {
> +#if HAVE_LIBPROCPS
>  	PROCTAB *proc;
>  	proc_t *proc_info;
>  	int err = 0;
> @@ -1272,6 +1305,28 @@ int igt_terminate_process(int sig, const char *comm)
>  
>  	closeproc(proc);
>  	return err;
> +#elif HAVE_LIBPROC2
> +	enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD };
> +	struct pids_info *info = NULL;
> +	struct pids_stack *stack;
> +	char *pid_comm;
> +	int pid;
> +	int err = 0;
> +
> +	if (procps_pids_new(&info, Items, 2) < 0)
> +		return -errno;
> +	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> +		pid = PIDS_VAL(0, s_int, stack, info);
> +		pid_comm = PIDS_VAL(1, str, stack, info);
> +		if (!strncasecmp(pid_comm, comm, strlen(pid_comm))) {
> +			if (kill(pid, sig) < 0)
> +				err = -errno;
> +			break;
> +		}
> +	}
> +	procps_pids_unref(&info);
> +	return err;
> +#endif
>  }
>  
>  struct pinfo {
> @@ -1341,9 +1396,9 @@ igt_show_stat_header(void)
>  }
>  
>  static void
> -igt_show_stat(proc_t *info, int *state, const char *fn)
> +igt_show_stat(const pid_t tid, const char *cmd, int *state, const char *fn)
>  {
> -	struct pinfo p = { .pid = info->tid, .comm = info->cmd, .fn = fn };
> +	struct pinfo p = { .pid = tid, .comm = cmd, .fn = fn };
>  
>  	if (!*state)
>  		igt_show_stat_header();
> @@ -1353,7 +1408,7 @@ igt_show_stat(proc_t *info, int *state, const char *fn)
>  }
>  
>  static void
> -__igt_lsof_fds(proc_t *proc_info, int *state, char *proc_path, const char *dir)
> +__igt_lsof_fds(const pid_t tid, const char *cmd, int *state, char *proc_path, const char *dir)
>  {
>  	struct dirent *d;
>  	struct stat st;
> @@ -1400,7 +1455,7 @@ again:
>  		dirn = dirname(copy_fd_lnk);
>  
>  		if (!strncmp(dir, dirn, strlen(dir)))
> -			igt_show_stat(proc_info, state, fd_lnk);
> +			igt_show_stat(tid, cmd, state, fd_lnk);
>  
>  		free(copy_fd_lnk);
>  		free(fd_lnk);
> @@ -1416,13 +1471,13 @@ again:
>  static void
>  __igt_lsof(const char *dir)
>  {
> -	PROCTAB *proc;
> -	proc_t *proc_info;
> -
>  	char path[30];
>  	char *name_lnk;
>  	struct stat st;
>  	int state = 0;
> +#ifdef HAVE_LIBPROCPS
> +	PROCTAB *proc;
> +	proc_t *proc_info;
>  
>  	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
>  	igt_assert(proc != NULL);
> @@ -1443,19 +1498,56 @@ __igt_lsof(const char *dir)
>  		name_lnk[read] = '\0';
>  
>  		if (!strncmp(dir, name_lnk, strlen(dir)))
> -			igt_show_stat(proc_info, &state, name_lnk);
> +			igt_show_stat(proc_info->tid, proc_info->cmd, &state, name_lnk);
>  
>  		/* check also fd, seems that lsof(8) doesn't look here */
>  		memset(path, 0, sizeof(path));
>  		snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid);
>  
> -		__igt_lsof_fds(proc_info, &state, path, dir);
> +		__igt_lsof_fds(proc_info->tid, proc_info->cmd, &state, path, dir);
>  
>  		free(name_lnk);
>  		freeproc(proc_info);
>  	}
>  
>  	closeproc(proc);
> +#elif HAVE_LIBPROC2
> +	enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD };
> +	struct pids_info *info = NULL;
> +	struct pids_stack *stack;
> +
> +	if (procps_pids_new(&info, Items, 2) < 0)
> +		return;
> +	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> +		ssize_t read;
> +		int tid = PIDS_VAL(0, s_int, stack, info);
> +		char *pid_comm = PIDS_VAL(1, str, stack, info);
> +
> +		/* check current working directory */
> +		memset(path, 0, sizeof(path));
> +		snprintf(path, sizeof(path), "/proc/%d/cwd", tid);
> +
> +		if (stat(path, &st) == -1)
> +			continue;
> +
> +		name_lnk = malloc(st.st_size + 1);
> +
> +		igt_assert((read = readlink(path, name_lnk, st.st_size + 1)));
> +		name_lnk[read] = '\0';
> +
> +		if (!strncmp(dir, name_lnk, strlen(dir)))
> +			igt_show_stat(tid, pid_comm, &state, name_lnk);
> +
> +		/* check also fd, seems that lsof(8) doesn't look here */
> +		memset(path, 0, sizeof(path));
> +		snprintf(path, sizeof(path), "/proc/%d/fd", tid);
> +
> +		__igt_lsof_fds(tid, pid_comm, &state, path, dir);
> +
> +		free(name_lnk);
> +	}
> +	procps_pids_unref(&info);
> +#endif
>  }
>  
>  /**
> @@ -1490,7 +1582,7 @@ igt_lsof(const char *dpath)
>  	free(sanitized);
>  }
>  
> -static void pulseaudio_unload_module(proc_t *proc_info)
> +static void pulseaudio_unload_module(const uid_t euid, const gid_t egid)
>  {
>  	struct igt_helper_process pa_proc = {};
>  	char xdg_dir[PATH_MAX];
> @@ -1498,14 +1590,14 @@ static void pulseaudio_unload_module(proc_t *proc_info)
>  	struct passwd *pw;
>  
>  	igt_fork_helper(&pa_proc) {
> -		pw = getpwuid(proc_info->euid);
> +		pw = getpwuid(euid);
>  		homedir = pw->pw_dir;
> -		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
> +		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", euid);
>  
>  		igt_info("Request pulseaudio to stop using audio device\n");
>  
> -		setgid(proc_info->egid);
> -		setuid(proc_info->euid);
> +		setgid(egid);
> +		setuid(euid);
>  		clearenv();
>  		setenv("HOME", homedir, 1);
>  		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
> @@ -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) {
------- ^
See below for libproc2 code.

> +		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,43 @@ static void pipewire_reserve_wait(void)
>  		}
>  		closeproc(proc);
>  
> +		tid = proc_info->tid;
> +		euid = proc_info->euid;
> +		egid = proc_info->egid;
> +		freeproc(proc_info);
> +#elif HAVE_LIBPROC2
> +	igt_fork(child, 1) {
------- ^
These are not the same constructs, +cc Petri.

Regards,
Kamil

> +		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;
> +		while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> +			tid = PIDS_VAL(EU_PID, s_int, stack, info);
> +			if (pipewire_pulse_pid == tid)
> +				break;
> +		}
> +		euid = PIDS_VAL(EU_EUID, s_int, stack, info);
> +		egid = PIDS_VAL(EU_EGID, s_int, stack, info);
> +		procps_pids_unref(&info);
> +#endif
> +
>  		/* Sanity check: if it can't find the process, it means it has gone */
> -		if (pipewire_pulse_pid != proc_info->tid)
> +		if (pipewire_pulse_pid != tid)
>  			exit(0);
>  
> -		pw = getpwuid(proc_info->euid);
> +		pw = getpwuid(euid);
>  		homedir = pw->pw_dir;
> -		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
> -		setgid(proc_info->egid);
> -		setuid(proc_info->euid);
> +		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", euid);
> +		setgid(egid);
> +		setuid(euid);
>  		clearenv();
>  		setenv("HOME", homedir, 1);
>  		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
> -		freeproc(proc_info);
>  
>  		/*
>  		 * pw-reserve will run in background. It will only exit when
> @@ -1570,9 +1689,7 @@ static void pipewire_reserve_wait(void)
>  int pipewire_pulse_start_reserve(void)
>  {
>  	bool is_pw_reserve_running = false;
> -	proc_t *proc_info;
>  	int attempts = 0;
> -	PROCTAB *proc;
>  
>  	if (!pipewire_pulse_pid)
>  		return 0;
> @@ -1584,6 +1701,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 +1719,24 @@ int pipewire_pulse_start_reserve(void)
>  			freeproc(proc_info);
>  		}
>  		closeproc(proc);
> +#elif 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;
> +		while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> +			if (!strcmp(PIDS_VAL(1, str, stack, info), "pw-reserve")) {
> +				is_pw_reserve_running = true;
> +				pipewire_pw_reserve_pid = PIDS_VAL(0, s_int, stack, info);
> +				break;
> +			}
> +		}
> +		procps_pids_unref(&info);
> +#endif
>  		if (is_pw_reserve_running)
>  			break;
>  	}
> @@ -1645,7 +1784,7 @@ void pipewire_pulse_stop_reserve(void)
>   * If the check fails, it means that the process can simply be killed.
>   */
>  static int
> -__igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
> +__igt_lsof_audio_and_kill_proc(const pid_t tid, const char *cmd, const uid_t euid, const gid_t egid, char *proc_path)
>  {
>  	const char *audio_dev = "/dev/snd/";
>  	char path[PATH_MAX * 2];
> @@ -1670,10 +1809,10 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
>  	 * 2) unload/unbind the the audio driver(s);
>  	 * 3) stop the pw-reserve thread.
>  	 */
> -	if (!strcmp(proc_info->cmd, "pipewire-pulse")) {
> +	if (!strcmp(cmd, "pipewire-pulse")) {
>  		igt_info("process %d (%s) is using audio device. Should be requested to stop using them.\n",
> -			 proc_info->tid, proc_info->cmd);
> -		pipewire_pulse_pid = proc_info->tid;
> +			 tid, cmd);
> +		pipewire_pulse_pid = tid;
>  		return 0;
>  	}
>  	/*
> @@ -1685,9 +1824,9 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
>  	 * will respawn them. So, just ignore here, they'll honor pw-reserve,
>  	 * when the time comes.
>  	 */
> -	if (!strcmp(proc_info->cmd, "pipewire-media-session"))
> +	if (!strcmp(cmd, "pipewire-media-session"))
>  		return 0;
> -	if (!strcmp(proc_info->cmd, "wireplumber"))
> +	if (!strcmp(cmd, "wireplumber"))
>  		return 0;
>  
>  	dp = opendir(proc_path);
> @@ -1723,22 +1862,22 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
>  		 * enough to unbind audio modules and won't cause race issues
>  		 * with systemd trying to reload it.
>  		 */
> -		if (!strcmp(proc_info->cmd, "pulseaudio")) {
> -			pulseaudio_unload_module(proc_info);
> +		if (!strcmp(cmd, "pulseaudio")) {
> +			pulseaudio_unload_module(euid, egid);
>  			break;
>  		}
>  
>  		/* For all other processes, just kill them */
>  		igt_info("process %d (%s) is using audio device. Should be terminated.\n",
> -				proc_info->tid, proc_info->cmd);
> +				tid, cmd);
>  
> -		if (kill(proc_info->tid, SIGTERM) < 0) {
> +		if (kill(tid, SIGTERM) < 0) {
>  			igt_info("Fail to terminate %s (pid: %d) with SIGTERM\n",
> -				proc_info->cmd, proc_info->tid);
> -			if (kill(proc_info->tid, SIGABRT) < 0) {
> +				cmd, tid);
> +			if (kill(tid, SIGABRT) < 0) {
>  				fail++;
>  				igt_info("Fail to terminate %s (pid: %d) with SIGABRT\n",
> -					proc_info->cmd, proc_info->tid);
> +					cmd, tid);
>  			}
>  		}
>  
> @@ -1760,23 +1899,47 @@ int
>  igt_lsof_kill_audio_processes(void)
>  {
>  	char path[PATH_MAX];
> +	int fail = 0;
> +
> +#ifdef HAVE_LIBPROCPS
>  	proc_t *proc_info;
>  	PROCTAB *proc;
> -	int fail = 0;
>  
>  	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
>  	igt_assert(proc != NULL);
>  	pipewire_pulse_pid = 0;
> -
>  	while ((proc_info = readproc(proc, NULL))) {
>  		if (snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid) < 1)
>  			fail++;
>  		else
> -			fail += __igt_lsof_audio_and_kill_proc(proc_info, path);
> +			fail += __igt_lsof_audio_and_kill_proc(proc_info->tid, proc_info->cmd, proc_info->euid, proc_info->egid, path);
>  
>  		freeproc(proc_info);
>  	}
>  	closeproc(proc);
> +#elif HAVE_LIBPROC2
> +	enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD, PIDS_ID_EUID, PIDS_ID_EGID };
> +	enum rel_items { EU_PID, EU_CMD, EU_EUID, EU_EGID };
> +	struct pids_info *info = NULL;
> +	struct pids_stack *stack;
> +	pid_t tid;
> +
> +	if (procps_pids_new(&info, Items, 4) < 0)
> +		return 1;
> +	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> +		tid = PIDS_VAL(EU_PID, s_int, stack, info);
> +
> +		if (snprintf(path, sizeof(path), "/proc/%d/fd", tid) < 1)
> +			fail++;
> +		else
> +			fail += __igt_lsof_audio_and_kill_proc(tid,
> +				PIDS_VAL(EU_CMD, str, stack, info),
> +				PIDS_VAL(EU_EUID, s_int, stack, info),
> +				PIDS_VAL(EU_EGID, s_int, stack, info),
> +				path);
> +	}
> +	procps_pids_unref(&info);
> +#endif
>  
>  	return fail;
>  }
> diff --git a/lib/meson.build b/lib/meson.build
> index b21c252b5..48a27a612 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -113,7 +113,6 @@ lib_deps = [
>  	libdrm,
>  	libdw,
>  	libkmod,
> -	libprocps,
>  	libudev,
>  	math,
>  	pciaccess,
> @@ -177,6 +176,12 @@ if chamelium.found()
>  	lib_sources += 'monitor_edids/monitor_edids_helper.c'
>  endif
>  
> +if libprocps.found()
> +	lib_deps += libprocps
> +else
> +	lib_deps += libproc2
> +endif
> +
>  if get_option('srcdir') != ''
>      srcdir = join_paths(get_option('srcdir'), 'tests')
>  else
> diff --git a/meson.build b/meson.build
> index 036217844..98acc8daf 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -124,7 +124,15 @@ build_info += 'With libdrm: ' + ','.join(libdrm_info)
>  
>  pciaccess = dependency('pciaccess', version : '>=0.10')
>  libkmod = dependency('libkmod')
> -libprocps = dependency('libprocps', required : true)
> +libprocps = dependency('libprocps', required : false)
> +libproc2 = dependency('libproc2', required : false)
> +if libprocps.found()
> +  config.set('HAVE_LIBPROCPS', 1)
> +elif libproc2.found()
> +  config.set('HAVE_LIBPROC2', 1)
> +else
> +  error('Either libprocps or libproc2 is required')
> +endif
>  
>  libunwind = dependency('libunwind', required : get_option('libunwind'))
>  build_info += 'With libunwind: @0@'.format(libunwind.found())
> -- 
> 2.37.2
> 

Re: [igt-dev] [PATCH i-g-t 1/1] Use the new procps library libproc2

[Mauro Carvalho Chehab]

On Tue, 25 Apr 2023 10:35:54 +0200
Kamil Konieczny <kamil.konieczny@linux.intel.com> wrote:

> Hi,
> 
> I forgot that there is one place which possibly
> need a rework, see below.
> 
> On 2023-04-24 at 18:26:42 +0200, Kamil Konieczny wrote:
> > From: Craig Small <csmall@dropbear.xyz>
> > 
> > Added support for new libproc2.
> > 
> > [Corrected some errors pointed by checkpatch.pl,
> >  add linux includes in #ifdef __linux__ section,
> >  use #elif HAVE_LIBPROC2 for new libproc2 code - Kamil]
> > Signed-off-by: Craig Small <csmall@dropbear.xyz>
> > ---
> >  lib/igt_aux.c   | 243 ++++++++++++++++++++++++++++++++++++++++--------
> >  lib/meson.build |   7 +-
> >  meson.build     |  10 +-
> >  3 files changed, 218 insertions(+), 42 deletions(-)
> > 
> > diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> > index 672d7d4b0..db6c4da93 100644
> > --- a/lib/igt_aux.c
> > +++ b/lib/igt_aux.c
> > @@ -52,8 +52,19 @@
> >  #include <assert.h>
> >  #include <grp.h>
> >  
> > +
> >  #include <proc/readproc.h>
> > +#elif HAVE_LIBPROC2
> > +#include <libproc2/pids.h>
> > +#endif

As IGT needs either one or the other, I would do, instead:

#ifdef HAVE_LIBPROCPS
#  include <proc/readproc.h>
#else
#  include <libproc2/pids.h>
#endif

Same applies to the other similar checks. 

The rationale is that, if something goes wrong at meson.build setting,
we want build to crash due to the lack of procps-ng.

> > +
> > +#include <dirent.h>
> > +#ifdef __linux__
> >  #include <libudev.h>
> > +#include <linux/limits.h>
> > +#else
> > +#include <limits.h>
> > +#endif

At least part of the changes here seem to be unrelated.

> >  
> >  #include "drmtest.h"
> >  #include "i915_drm.h"
> > @@ -1217,6 +1228,7 @@ void igt_unlock_mem(void)
> >   */
> >  int igt_is_process_running(const char *comm)
> >  {
> > +#if HAVE_LIBPROCPS
> >  	PROCTAB *proc;
> >  	proc_t *proc_info;
> >  	bool found = false;
> > @@ -1235,6 +1247,26 @@ int igt_is_process_running(const char *comm)
> >  
> >  	closeproc(proc);
> >  	return found;
> > +#elif HAVE_LIBPROC2
> > +	enum pids_item Item[] = { PIDS_CMD };
> > +	struct pids_info *info = NULL;
> > +	struct pids_stack *stack;
> > +	char *pid_comm;
> > +	bool found = false;
> > +
> > +	if (procps_pids_new(&info, Item, 1) < 0)
> > +		return false;
> > +	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> > +		pid_comm = PIDS_VAL(0, str, stack, info);
> > +		if (!strncasecmp(pid_comm, comm, strlen(pid_comm))) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	procps_pids_unref(&info);
> > +	return found;
> > +#endif
> >  }
> >  
> >  /**
> > @@ -1251,6 +1283,7 @@ int igt_is_process_running(const char *comm)
> >   */
> >  int igt_terminate_process(int sig, const char *comm)
> >  {
> > +#if HAVE_LIBPROCPS
> >  	PROCTAB *proc;
> >  	proc_t *proc_info;
> >  	int err = 0;
> > @@ -1272,6 +1305,28 @@ int igt_terminate_process(int sig, const char *comm)
> >  
> >  	closeproc(proc);
> >  	return err;
> > +#elif HAVE_LIBPROC2
> > +	enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD };
> > +	struct pids_info *info = NULL;
> > +	struct pids_stack *stack;
> > +	char *pid_comm;
> > +	int pid;
> > +	int err = 0;
> > +
> > +	if (procps_pids_new(&info, Items, 2) < 0)
> > +		return -errno;
> > +	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> > +		pid = PIDS_VAL(0, s_int, stack, info);
> > +		pid_comm = PIDS_VAL(1, str, stack, info);
> > +		if (!strncasecmp(pid_comm, comm, strlen(pid_comm))) {
> > +			if (kill(pid, sig) < 0)
> > +				err = -errno;
> > +			break;
> > +		}
> > +	}
> > +	procps_pids_unref(&info);
> > +	return err;
> > +#endif
> >  }
> >  
> >  struct pinfo {
> > @@ -1341,9 +1396,9 @@ igt_show_stat_header(void)
> >  }
> >  
> >  static void
> > -igt_show_stat(proc_t *info, int *state, const char *fn)
> > +igt_show_stat(const pid_t tid, const char *cmd, int *state, const char *fn)
> >  {
> > -	struct pinfo p = { .pid = info->tid, .comm = info->cmd, .fn = fn };
> > +	struct pinfo p = { .pid = tid, .comm = cmd, .fn = fn };
> >  
> >  	if (!*state)
> >  		igt_show_stat_header();
> > @@ -1353,7 +1408,7 @@ igt_show_stat(proc_t *info, int *state, const char *fn)
> >  }
> >  
> >  static void
> > -__igt_lsof_fds(proc_t *proc_info, int *state, char *proc_path, const char *dir)
> > +__igt_lsof_fds(const pid_t tid, const char *cmd, int *state, char *proc_path, const char *dir)
> >  {
> >  	struct dirent *d;
> >  	struct stat st;
> > @@ -1400,7 +1455,7 @@ again:
> >  		dirn = dirname(copy_fd_lnk);
> >  
> >  		if (!strncmp(dir, dirn, strlen(dir)))
> > -			igt_show_stat(proc_info, state, fd_lnk);
> > +			igt_show_stat(tid, cmd, state, fd_lnk);
> >  
> >  		free(copy_fd_lnk);
> >  		free(fd_lnk);
> > @@ -1416,13 +1471,13 @@ again:
> >  static void
> >  __igt_lsof(const char *dir)
> >  {
> > -	PROCTAB *proc;
> > -	proc_t *proc_info;
> > -
> >  	char path[30];
> >  	char *name_lnk;
> >  	struct stat st;
> >  	int state = 0;
> > +#ifdef HAVE_LIBPROCPS
> > +	PROCTAB *proc;
> > +	proc_t *proc_info;
> >  
> >  	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
> >  	igt_assert(proc != NULL);
> > @@ -1443,19 +1498,56 @@ __igt_lsof(const char *dir)
> >  		name_lnk[read] = '\0';
> >  
> >  		if (!strncmp(dir, name_lnk, strlen(dir)))
> > -			igt_show_stat(proc_info, &state, name_lnk);
> > +			igt_show_stat(proc_info->tid, proc_info->cmd, &state, name_lnk);
> >  
> >  		/* check also fd, seems that lsof(8) doesn't look here */
> >  		memset(path, 0, sizeof(path));
> >  		snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid);
> >  
> > -		__igt_lsof_fds(proc_info, &state, path, dir);
> > +		__igt_lsof_fds(proc_info->tid, proc_info->cmd, &state, path, dir);
> >  
> >  		free(name_lnk);
> >  		freeproc(proc_info);
> >  	}
> >  
> >  	closeproc(proc);
> > +#elif HAVE_LIBPROC2
> > +	enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD };
> > +	struct pids_info *info = NULL;
> > +	struct pids_stack *stack;
> > +
> > +	if (procps_pids_new(&info, Items, 2) < 0)
> > +		return;
> > +	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> > +		ssize_t read;
> > +		int tid = PIDS_VAL(0, s_int, stack, info);
> > +		char *pid_comm = PIDS_VAL(1, str, stack, info);
> > +
> > +		/* check current working directory */
> > +		memset(path, 0, sizeof(path));
> > +		snprintf(path, sizeof(path), "/proc/%d/cwd", tid);
> > +
> > +		if (stat(path, &st) == -1)
> > +			continue;
> > +
> > +		name_lnk = malloc(st.st_size + 1);
> > +
> > +		igt_assert((read = readlink(path, name_lnk, st.st_size + 1)));
> > +		name_lnk[read] = '\0';
> > +
> > +		if (!strncmp(dir, name_lnk, strlen(dir)))
> > +			igt_show_stat(tid, pid_comm, &state, name_lnk);
> > +
> > +		/* check also fd, seems that lsof(8) doesn't look here */
> > +		memset(path, 0, sizeof(path));
> > +		snprintf(path, sizeof(path), "/proc/%d/fd", tid);
> > +
> > +		__igt_lsof_fds(tid, pid_comm, &state, path, dir);
> > +
> > +		free(name_lnk);
> > +	}
> > +	procps_pids_unref(&info);
> > +#endif
> >  }
> >  
> >  /**
> > @@ -1490,7 +1582,7 @@ igt_lsof(const char *dpath)
> >  	free(sanitized);
> >  }
> >  
> > -static void pulseaudio_unload_module(proc_t *proc_info)
> > +static void pulseaudio_unload_module(const uid_t euid, const gid_t egid)
> >  {
> >  	struct igt_helper_process pa_proc = {};
> >  	char xdg_dir[PATH_MAX];
> > @@ -1498,14 +1590,14 @@ static void pulseaudio_unload_module(proc_t *proc_info)
> >  	struct passwd *pw;
> >  
> >  	igt_fork_helper(&pa_proc) {
> > -		pw = getpwuid(proc_info->euid);
> > +		pw = getpwuid(euid);
> >  		homedir = pw->pw_dir;
> > -		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
> > +		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", euid);
> >  
> >  		igt_info("Request pulseaudio to stop using audio device\n");
> >  
> > -		setgid(proc_info->egid);
> > -		setuid(proc_info->euid);
> > +		setgid(egid);
> > +		setuid(euid);
> >  		clearenv();
> >  		setenv("HOME", homedir, 1);
> >  		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
> > @@ -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) {  
> ------- ^
> See below for libproc2 code.
> 
> > +		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,43 @@ static void pipewire_reserve_wait(void)
> >  		}
> >  		closeproc(proc);
> >  
> > +		tid = proc_info->tid;
> > +		euid = proc_info->euid;
> > +		egid = proc_info->egid;
> > +		freeproc(proc_info);
> > +#elif HAVE_LIBPROC2
> > +	igt_fork(child, 1) {  
> ------- ^
> These are not the same constructs, +cc Petri.

I'm almost sure you need to use igt_fork_helper() here too. There are
some restrictions with igt_fork() that may affect the code to run on
some situations:

        internal_assert(!test_with_subtests || in_subtest,
                        "forking is only allowed in subtests or igt_simple_main\n");
        internal_assert(!test_child,
                        "forking is not allowed from already forked children\n");

Such restrictions don't apply to igt_fork_helper().

> 
> Regards,
> Kamil
> 
> > +		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;
> > +		while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> > +			tid = PIDS_VAL(EU_PID, s_int, stack, info);
> > +			if (pipewire_pulse_pid == tid)
> > +				break;
> > +		}
> > +		euid = PIDS_VAL(EU_EUID, s_int, stack, info);
> > +		egid = PIDS_VAL(EU_EGID, s_int, stack, info);
> > +		procps_pids_unref(&info);
> > +#endif

I would try to confine the procps-ng version check to the helper functions,
keeping here a logic that would be generic enough to be used with either
versions, probably using some abstraction, like placing version-specific
structs like:

	#ifdef HAVE_LIBPROCPS
		struct igt_process {
			struct pids_info *info = NULL;
			struct pids_stack *stack;
		};
	#else
		struct igt_process {
			proc_t *proc_info;
			PROCTAB *proc;
		};
	#endif

And adding a helper functions to handle open/close and ID retrieve logic,
e.g. changing the code to something similar to:

	struct igt_process proc;

	...
	igt_info("Preventing pipewire-pulse to use the audio drivers\n");
	open_process(&proc);
	while (get_process_ids(proc, &tid, &euid, &egid)) {
		if (pipewire_pulse_pid == tid)
			break;
	}
	close_proccess(proc);

This would make easier to maintain, as newer versions of procps-ng
may use different implementations too, and the business logic will
be split from the procps API internal details.

> > +
> >  		/* Sanity check: if it can't find the process, it means it has gone */
> > -		if (pipewire_pulse_pid != proc_info->tid)
> > +		if (pipewire_pulse_pid != tid)
> >  			exit(0);
> >  
> > -		pw = getpwuid(proc_info->euid);
> > +		pw = getpwuid(euid);
> >  		homedir = pw->pw_dir;
> > -		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
> > -		setgid(proc_info->egid);
> > -		setuid(proc_info->euid);
> > +		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", euid);
> > +		setgid(egid);
> > +		setuid(euid);
> >  		clearenv();
> >  		setenv("HOME", homedir, 1);
> >  		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
> > -		freeproc(proc_info);
> >  
> >  		/*
> >  		 * pw-reserve will run in background. It will only exit when
> > @@ -1570,9 +1689,7 @@ static void pipewire_reserve_wait(void)
> >  int pipewire_pulse_start_reserve(void)
> >  {
> >  	bool is_pw_reserve_running = false;
> > -	proc_t *proc_info;
> >  	int attempts = 0;
> > -	PROCTAB *proc;
> >  
> >  	if (!pipewire_pulse_pid)
> >  		return 0;
> > @@ -1584,6 +1701,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 +1719,24 @@ int pipewire_pulse_start_reserve(void)
> >  			freeproc(proc_info);
> >  		}
> >  		closeproc(proc);
> > +#elif 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;
> > +		while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> > +			if (!strcmp(PIDS_VAL(1, str, stack, info), "pw-reserve")) {
> > +				is_pw_reserve_running = true;
> > +				pipewire_pw_reserve_pid = PIDS_VAL(0, s_int, stack, info);
> > +				break;
> > +			}
> > +		}
> > +		procps_pids_unref(&info);
> > +#endif

Same comment as above: better to abstract API differences into a set
of open, close and get ID functions.

> >  		if (is_pw_reserve_running)
> >  			break;
> >  	}
> > @@ -1645,7 +1784,7 @@ void pipewire_pulse_stop_reserve(void)
> >   * If the check fails, it means that the process can simply be killed.
> >   */
> >  static int
> > -__igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
> > +__igt_lsof_audio_and_kill_proc(const pid_t tid, const char *cmd, const uid_t euid, const gid_t egid, char *proc_path)
> >  {
> >  	const char *audio_dev = "/dev/snd/";
> >  	char path[PATH_MAX * 2];
> > @@ -1670,10 +1809,10 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
> >  	 * 2) unload/unbind the the audio driver(s);
> >  	 * 3) stop the pw-reserve thread.
> >  	 */
> > -	if (!strcmp(proc_info->cmd, "pipewire-pulse")) {
> > +	if (!strcmp(cmd, "pipewire-pulse")) {
> >  		igt_info("process %d (%s) is using audio device. Should be requested to stop using them.\n",
> > -			 proc_info->tid, proc_info->cmd);
> > -		pipewire_pulse_pid = proc_info->tid;
> > +			 tid, cmd);
> > +		pipewire_pulse_pid = tid;
> >  		return 0;
> >  	}
> >  	/*
> > @@ -1685,9 +1824,9 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
> >  	 * will respawn them. So, just ignore here, they'll honor pw-reserve,
> >  	 * when the time comes.
> >  	 */
> > -	if (!strcmp(proc_info->cmd, "pipewire-media-session"))
> > +	if (!strcmp(cmd, "pipewire-media-session"))
> >  		return 0;
> > -	if (!strcmp(proc_info->cmd, "wireplumber"))
> > +	if (!strcmp(cmd, "wireplumber"))
> >  		return 0;
> >  
> >  	dp = opendir(proc_path);
> > @@ -1723,22 +1862,22 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
> >  		 * enough to unbind audio modules and won't cause race issues
> >  		 * with systemd trying to reload it.
> >  		 */
> > -		if (!strcmp(proc_info->cmd, "pulseaudio")) {
> > -			pulseaudio_unload_module(proc_info);
> > +		if (!strcmp(cmd, "pulseaudio")) {
> > +			pulseaudio_unload_module(euid, egid);
> >  			break;
> >  		}
> >  
> >  		/* For all other processes, just kill them */
> >  		igt_info("process %d (%s) is using audio device. Should be terminated.\n",
> > -				proc_info->tid, proc_info->cmd);
> > +				tid, cmd);
> >  
> > -		if (kill(proc_info->tid, SIGTERM) < 0) {
> > +		if (kill(tid, SIGTERM) < 0) {
> >  			igt_info("Fail to terminate %s (pid: %d) with SIGTERM\n",
> > -				proc_info->cmd, proc_info->tid);
> > -			if (kill(proc_info->tid, SIGABRT) < 0) {
> > +				cmd, tid);
> > +			if (kill(tid, SIGABRT) < 0) {
> >  				fail++;
> >  				igt_info("Fail to terminate %s (pid: %d) with SIGABRT\n",
> > -					proc_info->cmd, proc_info->tid);
> > +					cmd, tid);
> >  			}
> >  		}
> >  
> > @@ -1760,23 +1899,47 @@ int
> >  igt_lsof_kill_audio_processes(void)
> >  {
> >  	char path[PATH_MAX];
> > +	int fail = 0;
> > +
> > +#ifdef HAVE_LIBPROCPS
> >  	proc_t *proc_info;
> >  	PROCTAB *proc;
> > -	int fail = 0;
> >  
> >  	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
> >  	igt_assert(proc != NULL);
> >  	pipewire_pulse_pid = 0;
> > -
> >  	while ((proc_info = readproc(proc, NULL))) {
> >  		if (snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid) < 1)
> >  			fail++;
> >  		else
> > -			fail += __igt_lsof_audio_and_kill_proc(proc_info, path);
> > +			fail += __igt_lsof_audio_and_kill_proc(proc_info->tid, proc_info->cmd, proc_info->euid, proc_info->egid, path);
> >  
> >  		freeproc(proc_info);
> >  	}
> >  	closeproc(proc);
> > +#elif HAVE_LIBPROC2
> > +	enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD, PIDS_ID_EUID, PIDS_ID_EGID };
> > +	enum rel_items { EU_PID, EU_CMD, EU_EUID, EU_EGID };
> > +	struct pids_info *info = NULL;
> > +	struct pids_stack *stack;
> > +	pid_t tid;
> > +
> > +	if (procps_pids_new(&info, Items, 4) < 0)
> > +		return 1;
> > +	while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) {
> > +		tid = PIDS_VAL(EU_PID, s_int, stack, info);
> > +
> > +		if (snprintf(path, sizeof(path), "/proc/%d/fd", tid) < 1)
> > +			fail++;
> > +		else
> > +			fail += __igt_lsof_audio_and_kill_proc(tid,
> > +				PIDS_VAL(EU_CMD, str, stack, info),
> > +				PIDS_VAL(EU_EUID, s_int, stack, info),
> > +				PIDS_VAL(EU_EGID, s_int, stack, info),
> > +				path);
> > +	}
> > +	procps_pids_unref(&info);
> > +#endif

Same here: better to use API-abstract functions.

> >  
> >  	return fail;
> >  }
> > diff --git a/lib/meson.build b/lib/meson.build
> > index b21c252b5..48a27a612 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -113,7 +113,6 @@ lib_deps = [
> >  	libdrm,
> >  	libdw,
> >  	libkmod,
> > -	libprocps,
> >  	libudev,
> >  	math,
> >  	pciaccess,
> > @@ -177,6 +176,12 @@ if chamelium.found()
> >  	lib_sources += 'monitor_edids/monitor_edids_helper.c'
> >  endif
> >  
> > +if libprocps.found()
> > +	lib_deps += libprocps
> > +else
> > +	lib_deps += libproc2
> > +endif
> > +
> >  if get_option('srcdir') != ''
> >      srcdir = join_paths(get_option('srcdir'), 'tests')
> >  else
> > diff --git a/meson.build b/meson.build
> > index 036217844..98acc8daf 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -124,7 +124,15 @@ build_info += 'With libdrm: ' + ','.join(libdrm_info)
> >  
> >  pciaccess = dependency('pciaccess', version : '>=0.10')
> >  libkmod = dependency('libkmod')
> > -libprocps = dependency('libprocps', required : true)
> > +libprocps = dependency('libprocps', required : false)
> > +libproc2 = dependency('libproc2', required : false)
> > +if libprocps.found()
> > +  config.set('HAVE_LIBPROCPS', 1)
> > +elif libproc2.found()
> > +  config.set('HAVE_LIBPROC2', 1)
> > +else
> > +  error('Either libprocps or libproc2 is required')
> > +endif
> >  
> >  libunwind = dependency('libunwind', required : get_option('libunwind'))
> >  build_info += 'With libunwind: @0@'.format(libunwind.found())
> > -- 
> > 2.37.2
> >   

Re: [igt-dev] [PATCH i-g-t 1/1] Use the new procps library libproc2

[Mauro Carvalho Chehab]

On Fri, 28 Apr 2023 13:28:08 +0200
Mauro Carvalho Chehab <mauro.chehab@linux.intel.com> wrote:

> On Tue, 25 Apr 2023 10:35:54 +0200
> Kamil Konieczny <kamil.konieczny@linux.intel.com> wrote:
> 
> I would try to confine the procps-ng version check to the helper functions,
> keeping here a logic that would be generic enough to be used with either
> versions, probably using some abstraction, like placing version-specific
> structs like:
> 
> 	#ifdef HAVE_LIBPROCPS
> 		struct igt_process {
> 			struct pids_info *info = NULL;
> 			struct pids_stack *stack;
> 		};
> 	#else
> 		struct igt_process {
> 			proc_t *proc_info;
> 			PROCTAB *proc;
> 		};
> 	#endif
> 
> And adding a helper functions to handle open/close and ID retrieve logic,
> e.g. changing the code to something similar to:
> 
> 	struct igt_process proc;
> 
> 	...
> 	igt_info("Preventing pipewire-pulse to use the audio drivers\n");
> 	open_process(&proc);
> 	while (get_process_ids(proc, &tid, &euid, &egid)) {
> 		if (pipewire_pulse_pid == tid)
> 			break;
> 	}
> 	close_proccess(proc);
> 
> This would make easier to maintain, as newer versions of procps-ng
> may use different implementations too, and the business logic will
> be split from the procps API internal details.

As a generic note, #ifdefs usually makes the code harder to read.

So, I would try to code implementation inside the C source code either
as separate files or, if inside the same C file, as:


	#ifdef HAVE_LIBPROCPS
	// procps-ng (up to) version 3.x
	#  include <proc/readproc.h>
	#else
	// procps-ng version 4.x
	#  include <libproc2/pids.h>
	#endif

	...

 	#ifdef HAVE_LIBPROCPS

	struct igt_process {
		struct pids_info *info;
		struct pids_stack *stack;
	};

	int open_process(...)
	{
		// procps-ng version 3 implementation
	}

	void close_process(...)
	{
		// procps-ng version 3 implementation
	}

	int get_process_ids(...)
	{
		// procps-ng version 3 implementation
	}

	...

 	#else

	struct igt_process {
		proc_t *proc_info;
		PROCTAB *proc;
	};

	int open_process(...)
	{
		// procps-ng version 4 implementation
	}

	void close_process(...)
	{
		// procps-ng version 4 implementation
	}

	int get_process_ids(...)
	{
		// procps-ng version 4 implementation
	}

		...
 	#endif // HAVE_LIBPROCPS

Regards,
Mauro