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; Reviewed-by: Laine Stump <laine(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 5f112c775f..fb89b80da4 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -201,42 +201,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; --- Changes in v2: - remove virReportError() to avoid error-stacking per review from Erik src/node_device/node_device_driver.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 180d9da529..8227cd8342 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1207,9 +1207,14 @@ 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) + 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