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

[Vipin Sharma]

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.

> +	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?

> +
> +	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.

> +	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.

> +	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?

> +	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()?

> +	return out_driver;