On 4/13/21 4:49 PM, Jonathon Jongsma wrote:

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().

Seems like potato potahto to me (I guess you eliminated 4 lines), but sure, okay. Reviewed-by: Laine Stump <laine@redhat.com> \

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- 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);

Use of %i (instead of %d) seemed odd to me, so I looked it up to be sure and the two are synonyms in printf format strings, only have a different meaning for sscanf. We only use %i in an sscanf format string in one place, and use %i in printf format strings a handful of times. It may seem nitpicky, but I'd rather all the printf format strings were %d for consistency's sake. We could start in this direction by using %d here.

}

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; }

On 4/13/21 4:49 PM, Jonathon Jongsma wrote:

Coverity reported that this function can return NULL, so it should be handled properly.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

--- 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"),