Re: [libvirt] [PATCHv5 05/19] util: Refactor code for determining allocation path

> -----Original Message----- > From: John Ferlan [mailto:jferlan@redhat.com] > Sent: Wednesday, October 10, 2018 7:09 AM > To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com > Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>; > Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; > 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(a)intel.com&gt; >> --- >> 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; >>