Currently virMediatedDeviceGetIOMMUGroupDev() looks up the iommu group number and uses that to construct a path to the iommu group device. virMediatedDeviceGetIOMMUGroupNum() then uses that device path and takes the basename to get the group number. That's unnecessary extra string manipulation for *GroupNum(). Reverse the implementations and make *GroupDev() call *GroupNum(). Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/util/virmdev.c | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 102eb2bf67..8a5af49018 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -203,42 +203,38 @@ virMediatedDeviceGetPath(virMediatedDevice *dev) char * virMediatedDeviceGetIOMMUGroupDev(const char *uuidstr) { - g_autofree char *result_path = NULL; - g_autofree char *result_file = NULL; - g_autofree char *iommu_path = NULL; - g_autofree char *dev_path = virMediatedDeviceGetSysfsPath(uuidstr); - - iommu_path = g_strdup_printf("%s/iommu_group", dev_path); - - if (!virFileExists(iommu_path)) { - virReportSystemError(errno, _("failed to access '%s'"), iommu_path); - return NULL; - } + int group_num = virMediatedDeviceGetIOMMUGroupNum(uuidstr); - if (virFileResolveLink(iommu_path, &result_path) < 0) { - virReportSystemError(errno, _("failed to resolve '%s'"), iommu_path); + if (group_num < 0) return NULL; - } - - result_file = g_path_get_basename(result_path); - return g_strdup_printf("/dev/vfio/%s", result_file); + return g_strdup_printf("/dev/vfio/%i", group_num); } int virMediatedDeviceGetIOMMUGroupNum(const char *uuidstr) { - g_autofree char *vfio_path = NULL; + g_autofree char *result_path = NULL; g_autofree char *group_num_str = NULL; + g_autofree char *iommu_path = NULL; + g_autofree char *dev_path = virMediatedDeviceGetSysfsPath(uuidstr); unsigned int group_num = -1; - if (!(vfio_path = virMediatedDeviceGetIOMMUGroupDev(uuidstr))) + iommu_path = g_strdup_printf("%s/iommu_group", dev_path); + + if (!virFileExists(iommu_path)) { + virReportSystemError(errno, _("failed to access '%s'"), iommu_path); + return -1; + } + + if (virFileResolveLink(iommu_path, &result_path) < 0) { + virReportSystemError(errno, _("failed to resolve '%s'"), iommu_path); return -1; + } - group_num_str = g_path_get_basename(vfio_path); + group_num_str = g_path_get_basename(result_path); ignore_value(virStrToLong_ui(group_num_str, NULL, 10, &group_num)); - return group_num; } -- 2.26.3

Coverity reported that this function can return NULL, so it should be handled properly. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 5f8995172d..2fcebd065b 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1192,9 +1192,18 @@ nodeDeviceDestroy(virNodeDevicePtr device) * shouldn't try to remove the device. */ g_autofree char *vfiogroup = virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid); - VIR_AUTOCLOSE fd = open(vfiogroup, O_RDONLY); + VIR_AUTOCLOSE fd = -1; g_autofree char *errmsg = NULL; + if (!vfiogroup) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to determine iommu group for '%s'"), + def->name); + goto cleanup; + } + + fd = open(vfiogroup, O_RDONLY); + if (fd < 0 && errno == EBUSY) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to destroy '%s': device in use"), -- 2.26.3

On Tue, Apr 13, 2021 at 03:49:33PM -0500, Jonathon Jongsma wrote: This will lead to error stacking - virMediatedDeviceGetIOMMUGroupDev already reported an error for you at this point. Erik