Re: [igt-dev] [PATCH i-g-t 6/8] igt/params: use igt_params_set_save for igt_set_module_param*

Petri Latvala
2020-04-28T12:54:29Z

On Tue, Apr 21, 2020 at 07:17:23PM +0300, Juha-Pekka Heikkila wrote:
> Unify parameter access.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
> lib/igt_params.c | 133 +++++++++++++++++++++++++++++++++++------------
> 1 file changed, 100 insertions(+), 33 deletions(-)
>
> diff --git a/lib/igt_params.c b/lib/igt_params.c
> index 8de47b02..369d224c 100644
> --- a/lib/igt_params.c
> +++ b/lib/igt_params.c
> @@ -27,12 +27,14 @@
> #include <stdbool.h>
> #include <stdio.h>
> #include <sys/ioctl.h>
> +#include <sys/stat.h>
>
> #include <i915_drm.h>
>
> #include "igt_core.h"
> #include "igt_params.h"
> #include "igt_sysfs.h"
> +#include "igt_debugfs.h"
>
> struct module_param_data {
> char *path;
> @@ -107,15 +109,59 @@ static void igt_params_save(int dir, const char *path, const char *name)
> module_params = data;
> }
>
> -static int __igt_params_open(int device, char **outpath)
> +static int __igt_params_open(int device, char **outpath, const char *param)
> {
> int dir, params = -1;
> + struct stat buffer;
> + char searchname[64];
> + char searchpath[PATH_MAX];
> + char *foundname, *ctx;
>
> - dir = igt_sysfs_open(device);
> + dir = igt_debugfs_dir(device);
> if (dir >= 0) {
> - params = openat(dir,
> - "device/driver/module/parameters",
> - O_RDONLY);
> + int devname;
> +
> + devname = openat(dir, "name", O_RDONLY);
> + igt_assert(devname >= 0);
> +
> + read(devname, searchname, sizeof(searchname));
> + close(devname);
> +
> + foundname = strtok_r(searchname, " ", &ctx);
> + igt_assert(foundname);
> +
> + snprintf(searchpath, PATH_MAX, "%s_params", foundname);
> + params = openat(dir, searchpath, O_RDONLY);
> +
> + if (params >= 0) {
> + char *debugfspath = malloc(PATH_MAX);
> +
> + igt_debugfs_path(device, debugfspath, PATH_MAX);
> + if (param != NULL) {
> + char filepath[PATH_MAX];
> +
> + snprintf(filepath, PATH_MAX, "%s/%s",
> + debugfspath, param);
> +
> + if (stat(filepath, &buffer) == 0) {
> + if (outpath != NULL)
> + *outpath = debugfspath;
> + else
> + free(debugfspath);
> + } else {
> + free(debugfspath);
> + close(params);
> + params = -1;
> + }
> + } else if (outpath != NULL) {
> + /*
> + * Caller is responsible to free this.
> + */
> + *outpath = debugfspath;
> + } else {
> + free(debugfspath);
> + }
> + }
> close(dir);
> }
>
> @@ -124,11 +170,51 @@ static int __igt_params_open(int device, char **outpath)
> char name[32] = "";
> char path[PATH_MAX];
>
> - memset(&version, 0, sizeof(version));
> - version.name_len = sizeof(name);
> - version.name = name;
> - ioctl(device, DRM_IOCTL_VERSION, &version);
> -
> + if (device == -1) {
> + /*
> + * find default device
> + */

Oh, here's the defaulting.

Like stated in the replies for one of the other patches, I'm not
greatly fond of carrying around a concept of a "default" device. Are
there users of -1 today?

This is also considerably more code that one would expect from just
"use params_set_save" as the subject states.

> + int file, i;
> + const char *debugfs_root = igt_debugfs_mount();
> +
> + igt_assert(debugfs_root);
> +
> + for (i = 0; i < 63; i++) {
> + char testpath[PATH_MAX];
> +
> + snprintf(searchpath, PATH_MAX,
> + "%s/dri/%d/name", debugfs_root, i);
> +
> + file = open(searchpath, O_RDONLY);
> +
> + if (file < 0)
> + continue;
> +
> + read(file, searchname, sizeof(searchname));
> + close(file);
> +
> + foundname = strtok_r(searchname, " ", &ctx);
> + if (!foundname)
> + continue;
> +
> + snprintf(testpath, PATH_MAX,
> + "/sys/module/%s/parameters",
> + foundname);
> +
> + if (stat(testpath, &buffer) == 0 &&
> + S_ISDIR(buffer.st_mode)) {
> + memcpy(name, foundname,
> + (sizeof(name) < strlen(foundname) ?
> + sizeof(name) : strlen(foundname)));

If foundname is larger than the array, doesn't this leave 'name'
without a nul-termination?

--
Petri Latvala