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

[Raghavendra Rao Ananta]

On Mon, Mar 9, 2026 at 3:06 PM Vipin Sharma <vipinsh@xxxxxxxxxx> wrote:
>
> On 2026-03-03 19:38:17, Raghavendra Rao Ananta wrote:
> > +char *sysfs_sriov_vf_bdf_get(const char *pf_bdf, int i)
> > +{
> > +     char vf_path[PATH_MAX];
> > +     char path[PATH_MAX];
> > +     char *out_vf_bdf;
> > +     int ret;
> > +
> > +     out_vf_bdf = calloc(16, sizeof(char));
>
> A comment of /* ../0000:00:00.0 */ would be nice to tell why 16 is
> chosen.
>
Sure, will add it in v7.

> > +     VFIO_ASSERT_NOT_NULL(out_vf_bdf);
> > +
> > +     snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/virtfn%d", pf_bdf, i);
> > +
> > +     ret = readlink(path, vf_path, PATH_MAX);
> > +     VFIO_ASSERT_NE(ret, -1);
> > +     vf_path[ret] = '\0';
>
> Can we just initialize vf_path to {0} at the beginning and not worry
> here?
>
Hmm, we can but we don't want to fill 4K (PATH_MAX) worth of 0s for
every function call. Hence, I preferred to set it after the call to
readlink(). This also seems to be the common practise, at least by
looking in the "tools/" dir. With David's suggestion of introducing
readlink_safe(), the caller need not even worry about this. Same
response to the similar comments below.

> > +
> > +     ret = sscanf(basename(vf_path), "%s", out_vf_bdf);
> > +     VFIO_ASSERT_EQ(ret, 1);
> > +
> > +     return out_vf_bdf;
> > +}
> > +
> > +unsigned int sysfs_iommu_group_get(const char *bdf)
> > +{
> > +     char dev_iommu_group_path[PATH_MAX];
> > +     char path[PATH_MAX];
> > +     unsigned int group;
>
> Why unsigned int? In kernel iommu_group id is int itself. Caller of this
> function also casting it to int.
>
Ah, it should be 'int'. I guess I copied the original code as is. I'll
update it in v7.

> > +     int ret;
> > +
> > +     snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/iommu_group", bdf);
> > +
> > +     ret = readlink(path, dev_iommu_group_path, sizeof(dev_iommu_group_path));
> > +     VFIO_ASSERT_NE(ret, -1, "Failed to get the IOMMU group for device: %s\n", bdf);
> > +     dev_iommu_group_path[ret] = '\0';
>
> Same, initialize at beginning.
>
> > +
> > +     ret = sscanf(basename(dev_iommu_group_path), "%u", &group);
>
> Can we combine these operations into a single function? Seems like vfio,
> iommu_group and driver all use the same pattern.
>
I think we can, at least for the vf_driver and the iommu_group. The
'sysfs_driver_get' handles things slightly differently. I'll update in
v7.

> > +     VFIO_ASSERT_EQ(ret, 1, "Failed to get the IOMMU group for device: %s\n", bdf);
> > +
> > +     return group;
> > +}
> > +
> > +char *sysfs_driver_get(const char *bdf)
> > +{
> > +     char driver_path[PATH_MAX];
> > +     char path[PATH_MAX];
> > +     char *out_driver;
> > +     int ret;
> > +
> > +     out_driver = calloc(64, sizeof(char));
>
> Comment on why 64?
>
Will do in v7.

> > +     VFIO_ASSERT_NOT_NULL(out_driver);
> > +
> > +     snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/driver", bdf);
> > +     ret = readlink(path, driver_path, PATH_MAX);
> > +     if (ret == -1) {
> > +             free(out_driver);
> > +
> > +             if (errno == ENOENT)
> > +                     return NULL;
> > +
> > +             VFIO_FAIL("Failed to read %s\n", path);
> > +     }
> > +     driver_path[ret] = '\0';
>
> Same, initialize at beginning.
>
> > +
> > +     strcpy(out_driver, basename(driver_path));
>
> They both have different lengths. Should we use strncpy()?
>
The assumption was that 'out_driver' is allocated to accommodate a max
driver length and should fit any driver name. But I can switch to
strncpy() to make this more clear in v7.

Thank you.
Raghavendra