Re: [igt-dev] [PATCH i-g-t 3/8] lib/params: overhaul param saving

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 28.4.2020 21.40, Juha-Pekka Heikkila wrote:
> On 28.4.2020 15.26, Petri Latvala wrote:
>> On Tue, Apr 21, 2020 at 07:17:20PM +0300, Juha-Pekka Heikkila wrote:
>>> From: Jani Nikula <jani.nikula@intel.com>
>>>
>>> More generic, use existing functions.
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>> ---
>>>   lib/igt_params.c | 101 +++++++++++++++++++----------------------------
>>>   1 file changed, 41 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/lib/igt_params.c b/lib/igt_params.c
>>> index f220e73b..b5ac1266 100644
>>> --- a/lib/igt_params.c
>>> +++ b/lib/igt_params.c
>>> @@ -34,92 +34,74 @@
>>>   #include "igt_params.h"
>>>   #include "igt_sysfs.h"
>>> -#define MODULE_PARAM_DIR "/sys/module/i915/parameters/"
>>> -#define PARAM_NAME_MAX_SZ 32
>>> -#define PARAM_VALUE_MAX_SZ 16
>>> -#define PARAM_FILE_PATH_MAX_SZ (strlen(MODULE_PARAM_DIR) + 
>>> PARAM_NAME_MAX_SZ)
>>> -
>>>   struct module_param_data {
>>> -    char name[PARAM_NAME_MAX_SZ];
>>> -    char original_value[PARAM_VALUE_MAX_SZ];
>>> +    char *path;
>>> +    char *name;
>>> +    char *original_value;
>>>       struct module_param_data *next;
>>>   };
>>>   struct module_param_data *module_params = NULL;
>>> -static void igt_module_param_exit_handler(int sig)
>>> +static void igt_params_exit_handler(int sig)
>>>   {
>>> -    const size_t dir_len = strlen(MODULE_PARAM_DIR);
>>> -    char file_path[PARAM_FILE_PATH_MAX_SZ];
>>>       struct module_param_data *data;
>>> -    int fd;
>>> -
>>> -    /* We don't need to assert string sizes on this function since 
>>> they were
>>> -     * already checked before being stored on the lists. Besides,
>>> -     * igt_assert() is not AS-Safe. */
>>> -    strcpy(file_path, MODULE_PARAM_DIR);
>>> +    int dir;
>>>       for (data = module_params; data != NULL; data = data->next) {
>>> -        strcpy(file_path + dir_len, data->name);
>>> -
>>> -        fd = open(file_path, O_RDWR);
>>> -        if (fd >= 0) {
>>> -            int size = strlen (data->original_value);
>>> -
>>> -            if (size != write(fd, data->original_value, size)) {
>>> -                const char msg[] = "WARNING: Module parameters "
>>> -                    "may not have been reset to their "
>>> -                    "original values\n";
>>> -                assert(write(STDERR_FILENO, msg, sizeof(msg))
>>> -                       == sizeof(msg));
>>> -            }
>>> -
>>> -            close(fd);
>>> +        dir = open(data->path, O_RDONLY);
>>> +
>>> +        if (!igt_sysfs_set(dir, data->name, data->original_value)) {
>>> +            const char msg[] = "WARNING: Module parameters "
>>> +                "may not have been reset to their "
>>> +                "original values\n";
>>> +            assert(write(STDERR_FILENO, msg, sizeof(msg))
>>> +                   == sizeof(msg));
>>>           }
>>> +
>>> +        close(dir);
>>>       }
>>>       /* free() is not AS-Safe, so we can't call it here. */
>>>   }
>>>   /**
>>> - * igt_save_module_param:
>>> - * @name: name of the i915.ko module parameter
>>> - * @file_path: full sysfs file path for the parameter
>>> + * igt_params_save:
>>> + * @dir: file handle for path
>>> + * @path: full path to the sysfs directory
>>> + * @name: name of the sysfs attribute
>>>    *
>>> - * Reads the current value of an i915.ko module parameter, saves it 
>>> on an array,
>>> - * then installs an exit handler to restore it when the program exits.
>>> + * Reads the current value of a sysfs attribute, saves it on an 
>>> array, then
>>> + * installs an exit handler to restore it when the program exits.
>>>    *
>>>    * It is safe to call this function multiple times for the same 
>>> parameter.
>>>    *
>>>    * Notice that this function is called by igt_set_module_param(), 
>>> so that one -
>>>    * or one of its wrappers - is the only function the test programs 
>>> need to call.
>>>    */
>>> -static void igt_save_module_param(const char *name, const char 
>>> *file_path)
>>> +static void igt_params_save(int dir, const char *path, const char 
>>> *name)
>>>   {
>>>       struct module_param_data *data;
>>> -    size_t n;
>>> -    int fd;
>>>       /* Check if this parameter is already saved. */
>>>       for (data = module_params; data != NULL; data = data->next)
>>> -        if (strncmp(data->name, name, PARAM_NAME_MAX_SZ) == 0)
>>> +        if (strcmp(data->path, path) == 0 &&
>>> +            strcmp(data->name, name) == 0)
>>>               return;
>>>       if (!module_params)
>>> -        igt_install_exit_handler(igt_module_param_exit_handler);
>>> +        igt_install_exit_handler(igt_params_exit_handler);
>>>       data = calloc(1, sizeof (*data));
>>>       igt_assert(data);
>>> -    strncpy(data->name, name, PARAM_NAME_MAX_SZ - 1);
>>> -
>>> -    fd = open(file_path, O_RDONLY);
>>> -    igt_assert(fd >= 0);
>>> +    data->path = strdup(path);
>>> +    igt_assert(data->path);
>>> -    n = read(fd, data->original_value, PARAM_VALUE_MAX_SZ);
>>> -    igt_assert_f(n > 0 && n < PARAM_VALUE_MAX_SZ,
>>> -             "Need to increase PARAM_VALUE_MAX_SZ\n");
>>> +    data->name = strdup(name);
>>> +    igt_assert(data->name);
>>> -    igt_assert(close(fd) == 0);
>>> +    data->original_value = igt_sysfs_get(dir, name);
>>> +    igt_assert(data->original_value);
>>>       data->next = module_params;
>>>       module_params = data;
>>> @@ -205,22 +187,21 @@ bool igt_params_set(int device, const char 
>>> *parameter, const char *fmt, ...)
>>>    */
>>>   void igt_set_module_param(const char *name, const char *val)
>>>   {
>>> -    char file_path[PARAM_FILE_PATH_MAX_SZ];
>>> -    size_t len = strlen(val);
>>> -    int fd;
>>> +    const char *path = "/sys/module/i915/parameters";
>>> +    int dir;
>>> -    igt_assert_f(strlen(name) < PARAM_NAME_MAX_SZ,
>>> -             "Need to increase PARAM_NAME_MAX_SZ\n");
>>> -    strcpy(file_path, MODULE_PARAM_DIR);
>>> -    strcpy(file_path + strlen(MODULE_PARAM_DIR), name);
>>> +    dir = open(path, O_RDONLY);
>>> +    igt_assert(dir >= 0);
>>> -    igt_save_module_param(name, file_path);
>>> +    igt_params_save(dir, path, name);
>>> -    fd = open(file_path, O_RDWR);
>>> -    igt_assert(write(fd, val, len) == len);
>>> -    igt_assert(close(fd) == 0);
>>> +    igt_assert(igt_sysfs_set(dir, name, val));
>>> +
>>> +    igt_assert(close(dir) == 0);
>>>   }
>>> +#define PARAM_VALUE_MAX_SZ 16
>>> +
>>
>> Is this define still needed for something?
> 
> Look like it has moved inside igt_aux.c so it's not needed here, I'll 
> take it away.

Actually, it was used by code below it. It basically is assumption 
integer expands to string that will be PARAM_VALUE_MAX_SZ length at maximum.

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev