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

Raghavendra Rao Ananta
2026-04-07T22:46:27Z

On Mon, Apr 6, 2026 at 2:12 PM Alex Williamson <alex@shazbot.org> wrote:
>
> On Thu, 2 Apr 2026 17:30:54 +0000
> Raghavendra Rao Ananta <rananta@google.com> 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