[libvirt PATCH v2 1/3] qemu: remove unnecessary null check
virMediatedDeviceGetSysfsPath() (via g_strdup_printf()) is guaranteed to return a non-NULL value, so remove the unnecessary checks for NULL. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com> --- Changes in v2: - remove NULL checks from 2 additional locations src/qemu/qemu_command.c | 4 +--- src/util/virmdev.c | 7 +------ 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 83bebdd2a8..fbe387253e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5138,9 +5138,7 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def, g_autofree char *mdevPath = NULL; const char *dev_str = NULL; - if (!(mdevPath = virMediatedDeviceGetSysfsPath(mdevsrc->uuidstr))) - return NULL; - + mdevPath = virMediatedDeviceGetSysfsPath(mdevsrc->uuidstr); dev_str = qemuBuildHostdevMdevModelTypeString(mdevsrc); if (!dev_str) diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 7f4a499536..5f112c775f 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -140,9 +140,7 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) g_autoptr(virMediatedDevice) dev = NULL; g_autofree char *sysfspath = NULL; - if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr))) - return NULL; - + sysfspath = virMediatedDeviceGetSysfsPath(uuidstr); if (!virFileExists(sysfspath)) { virReportError(VIR_ERR_DEVICE_MISSING, _("mediated device '%s' not found"), uuidstr); @@ -208,9 +206,6 @@ virMediatedDeviceGetIOMMUGroupDev(const char *uuidstr) g_autofree char *iommu_path = NULL; g_autofree char *dev_path = virMediatedDeviceGetSysfsPath(uuidstr); - if (!dev_path) - return NULL; - iommu_path = g_strdup_printf("%s/iommu_group", dev_path); if (!virFileExists(iommu_path)) { -- 2.26.3
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@redhat.com> Reviewed-by: Laine Stump <laine@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 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
On Wed, Apr 14, 2021 at 03:54:48PM -0500, 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().
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
Coverity reported that this function can return NULL, so it should be handled properly. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- 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
On Wed, Apr 14, 2021 at 03:54:49PM -0500, 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: Erik Skultety <eskultet@redhat.com>
On Wed, Apr 14, 2021 at 03:54:47PM -0500, Jonathon Jongsma wrote:
virMediatedDeviceGetSysfsPath() (via g_strdup_printf()) is guaranteed to return a non-NULL value, so remove the unnecessary checks for NULL.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (2)
-
Erik Skultety -
Jonathon Jongsma