Re: [PATCH v7 3/8] vfio: selftests: Introduce a sysfs lib

[Raghavendra Rao Ananta]

On Mon, Apr 6, 2026 at 2:12 PM Alex Williamson <alex@xxxxxxxxxxx> wrote:
>
> On Thu,  2 Apr 2026 17:30:54 +0000
> Raghavendra Rao Ananta <rananta@xxxxxxxxxx> wrote:
> > diff --git a/tools/testing/selftests/vfio/lib/sysfs.c b/tools/testing/selftests/vfio/lib/sysfs.c
> > new file mode 100644
> > index 0000000000000..f1e6889556527
> > --- /dev/null
> > +++ b/tools/testing/selftests/vfio/lib/sysfs.c
> > @@ -0,0 +1,150 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <linux/limits.h>
> > +
> > +#include <libvfio.h>
> > +
> > +/* Enough to hold MODULE_NAME_LEN */
> > +#define DRIVER_NAME_SZ 64
> > +
> > +#define readlink_safe(_path, _buf) ({                                \
> > +     int __ret = readlink(_path, _buf, sizeof(_buf) - 1);    \
> > +     if (__ret != -1)                                        \
> > +             _buf[__ret] = 0;                                \
> > +     __ret;                                                  \
> > +})
>
> Not a current issue, but the use of sizeof() here is a danger for
> future users that might pass something other than a stack array.  We
> could avoid this by adding a build time test (untested):
>
>     _Static_assert(!__builtin_types_compatible_p(                       \
>                        __typeof__(_buf), char *),                       \
>                    "readlink_safe: _buf must be an array, not a pointer"); \
>
> Maybe overkill for an internal helper, but we could avoid the hazard.
>
Good point! It makes sense to have a build-time check for this. I will
incorporate the _Static_assert to avoid the hazard.

> ...
> > +char *sysfs_driver_get(const char *bdf)
> > +{
> > +     char driver_path[PATH_MAX];
> > +     char path[PATH_MAX];
> > +     char *out_driver;
> > +     int ret;
> > +
> > +     out_driver = calloc(DRIVER_NAME_SZ, sizeof(char));
> > +     VFIO_ASSERT_NOT_NULL(out_driver);
> > +
> > +     snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/driver", bdf);
> > +     ret = readlink_safe(path, driver_path);
> > +     if (ret == -1) {
> > +             free(out_driver);
> > +
> > +             if (errno == ENOENT)
> > +                     return NULL;
> > +
> > +             VFIO_FAIL("Failed to read %s\n", path);
> > +     }
> > +
> > +     strncpy(out_driver, basename(driver_path), DRIVER_NAME_SZ - 1);
>
> Nit, I'm not really sure what DRIVER_NAME_SZ buys us other than an
> artificial limit.  We're already bound by PATH_MAX and could just do a
> strdup() for the return.  Neither are blockers.  Thanks,
>
Ah, why didn't I think of strdup(). It's a better approach. I'll
update this in v8.

Thank you.
Raghavendra