-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, October 10, 2018 7:09 AM To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com Cc: Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>; Ding, Jian-feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@intel.com> Subject: Re: [libvirt] [PATCHv5 05/19] util: Refactor code for determining allocation path
On 10/9/18 6:30 AM, Wang Huaqiang wrote:
The code for determining resctrl allocation path could be reused for monitor. Refactor it for reusing.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/util/virresctrl.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index c9a79f7..03001cc 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2267,6 +2267,26 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, }
+static char * +virResctrlDeterminePath(const char *pathparent,
s/pathparent/parentpath
OK.
+ const char *prefix, + const char *id) { + char *path = NULL; + + if (!id) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Resctrl resource ID must be set before + creation"));
s/Resctrl resource ID/'%s' resctrl ID/
where %s is @parentpath
Working in the @parentpath, then we'd know which it was Alloc or Monitor
How about changes like these? if (!id) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Resctrl resource ID must be set before creation")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Resctrl ID must be set before determining resctrl " + "path under '%s'"), + parentpath); return NULL; }
+ return NULL; + } + + if (virAsprintf(&path, "%s/%s-%s", pathparent, prefix, id) < 0)
parentpath
Will be renamed.
+ return NULL; + + return path; +} + + int virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, const char *machinename) @@ -2274,15 +2294,16 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, if (!alloc) return 0;
- if (!alloc->id) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Resctrl Allocation ID must be set before creation")); + if (alloc->path) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Resctrl group path is expected to be + NULL"));
Resctrl alloc group path is already set '%s'
I think this is an internal error not an invalid arg, too
Will be changed. if (alloc->path) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Resctrl group path is expected to be NULL")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Resctrl allocation path is already set to '%s'"), + alloc->path); return -1; }
John
Thanks for review. Huaqiang
return -1; }
- if (!alloc->path && - virAsprintf(&alloc->path, "%s/%s-%s", - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0) + alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH, + machinename, + alloc->id); + if (!alloc->path) return -1;
return 0;