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

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Thursday, October 11, 2018 5:41 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/10/18 9:55 AM, Wang, Huaqiang wrote: > > >> -----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; > } > Seems reasonable... although instead of "path under", just go with "parentpath='%s'"

John > >> >>> + 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; >>> >