Re: [libvirt PATCH 2/3] nodedev: refactor virMediatedDeviceGetIOMMUGroupNum()
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(a)redhat.com>
\
Signed-off-by: Jonathon Jongsma <jjongsma(a)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;
}