Re: [PATCH i-g-t 1/2] lib/igt_configfs: Add library for configfs

[Riana Tauro <riana.tauro@intel.com>]

Hi Louis

On 4/11/2025 1:23 PM, Louis Chauvet wrote:
> Hi Riana,
> 
> Le 10/04/2025 à 08:07, Riana Tauro a écrit :
>> Add library functions to open configfs and create
>> and remove configfs directories.
> 
> Can you take a look at [1], something similar was proposed few weeks 
> ago? The main difference is that [1] checks that /sys/kernel/config is 
> actually a mount point.
I hadn't looked at this. Thank you for the link

@Jose is this close to merge? If not, can i send [2] and [3] as part of 
this series?

[2]https://lore.kernel.org/all/20250218165011.9123-3-jose.exposito89@gmail.com/
[3]https://lore.kernel.org/all/20250218165011.9123-4-jose.exposito89@gmail.com/> 

> +Cc: José Exposito
> 
> [1]:https://lore.kernel.org/all/20250218165011.9123-4- 
> jose.exposito89@gmail.com/
> 
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>>   lib/igt_configfs.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/igt_configfs.h | 13 +++++++
>>   lib/meson.build    |  1 +
>>   3 files changed, 102 insertions(+)
>>   create mode 100644 lib/igt_configfs.c
>>   create mode 100644 lib/igt_configfs.h
>>
>> diff --git a/lib/igt_configfs.c b/lib/igt_configfs.c
>> new file mode 100644
>> index 000000000..57d59f1c4
>> --- /dev/null
>> +++ b/lib/igt_configfs.c
>> @@ -0,0 +1,88 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +#include <sys/mount.h>
>> +#include <sys/stat.h>
>> +
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <limits.h>
>> +
>> +#include "igt_configfs.h"
>> +#include "igt_core.h"
>> +
>> +static const char *igt_configfs_mount(void)
>> +{
>> +    struct stat st;
>> +
>> +    if (stat("/sys/kernel/debug", &st) == 0)
>> +        return "/sys/kernel/config";
>> +
>> +    if (mount("none", "/sys/kernel/config", "configfs", 0, 0))
>> +        return NULL;
>> +
>> +    return "/sys/kernel/config";
>> +}
>> +
>> +/**
>> + * igt_configfs_open: open configfs path
>> + * @name: name of the configfs directory
>> + *
>> + * Opens the configfs directory corresponding to the name
>> + *
>> + * Returns:
>> + * The directory fd, or -1 on failure.
>> + */
>> +int igt_configfs_open(const char *name)
>> +{
>> +    char path[PATH_MAX];
>> +    const char *configfs_path;
>> +
>> +    configfs_path = igt_configfs_mount();
>> +    igt_assert(configfs_path);
>> +
>> +    snprintf(path, sizeof(path), "%s/%s", configfs_path, name);
>> +
>> +    return open(path, O_RDONLY);
>> +}
> 
> The usage of file descriptors for folders is nice here!
> 
> @José, I think this may reduce the complexity of the vkms tests. At 
> least it will avoid having snprintf(%s/%s, vkms_root, vkms_item). What 
> do you think?
> 
>> +
>> +/**
>> + * igt_configfs_create_directory: creates configfs group
>> + * @fd: fd of configfs parent directory
>> + * @name: name of the directory to create
>> + *
>> + * creates a directory under configfs parent directory
>> + *
>> + * Returns: 0 on success, -errno otherwise
> 
> This seems wrong, as the function returns a file descriptor, did I miss 
> something?
> Missed this. Will fix it>> + */
>> +int igt_configfs_create_directory(int fd, const char *name)
>> +{
>> +    int ret;
>> +    int dirfd;
>> +
>> +    ret = mkdirat(fd, name, 0755);
>> +    if (ret)
>> +        return -errno;
>> +
>> +    dirfd = openat(fd, name, O_DIRECTORY);
>> +    if (dirfd < 0)
>> +        return -errno;
>> +
>> +    return dirfd;
>> +}
>> +
>> +/**
>> + * igt_configfs_remove_directory: removes configfs group
>> + * @fd: fd of configfs parent directory
>> + * @name: name of directory to create
>> + *
>> + * removes directory under configfs parent directory
>> + */
>> +void igt_configfs_remove_directory(int fd, const char *name)
>> +{
>> +    int ret = unlinkat(fd, name, AT_REMOVEDIR);
>> +
>> +    if (ret)
>> +        igt_warn("Unable to remove %s directory: %s\n", name, 
>> strerror(errno));
>> +}
> 
> I completely understand the point of those helpers, but I don't think 
> they are configfs-specific. The file descriptor is passed by argument, 
> so you could use it for any interface (configfs, debugfs, sysfs...).
> 
> I don't see any igt_fs.c, do you think it beneficial to create such file?

I thought of calling the igt_configfs_open here but that would not work 
for nested configfs directories.

This could be moved to igt_io and that can be renamed to igt_fs as that 
has only file read and write calls

Thank you
Riana Tauro>
> @José, do you think it make sense to also reuse igt_sysfs helpers for 
> the VKMS tests? (igt_sysfs_get/set_s32/u32/boolean only use a directory 
> fd + path, if you decide to use fd for vkms, it will avoid code 
> duplication)
> 
> Thanks a lot Riana for this implementation and new ideas.
> 
> Louis Chauvet
> 
>> diff --git a/lib/igt_configfs.h b/lib/igt_configfs.h
>> new file mode 100644
>> index 000000000..d839db49a
>> --- /dev/null
>> +++ b/lib/igt_configfs.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#ifndef IGT_CONFIGFS_H
>> +#define IGT_CONFIGFS_H
>> +
>> +int igt_configfs_open(const char *name);
>> +int igt_configfs_create_directory(int fd, const char *name);
>> +void igt_configfs_remove_directory(int fd, const char *name);
>> +
>> +#endif /* IGT_CONFIGFS_H */
>> diff --git a/lib/meson.build b/lib/meson.build
>> index d7bb72c57..f087947e7 100644
>> --- a/lib/meson.build
>> +++ b/lib/meson.build
>> @@ -19,6 +19,7 @@ lib_sources = [
>>       'igt_collection.c',
>>       'igt_color_encoding.c',
>>       'igt_facts.c',
>> +    'igt_configfs.c',
>>       'igt_crc.c',
>>       'igt_debugfs.c',
>>       'igt_device.c',
>