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