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

[Petri Latvala <petri.latvala@intel.com>]

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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev