> -----Original Message----- > From: John Ferlan [mailto:jferlan@xxxxxxxxxx] > Sent: Thursday, October 11, 2018 5:41 AM > To: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx > Cc: Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing <bing.niu@xxxxxxxxx>; > Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui <rui.zang@xxxxxxxxx> > Subject: Re: [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@xxxxxxxxxx] > >> Sent: Wednesday, October 10, 2018 7:09 AM > >> To: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx > >> Cc: Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing > >> <bing.niu@xxxxxxxxx>; Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; > >> Zang, Rui <rui.zang@xxxxxxxxx> > >> Subject: Re: [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@xxxxxxxxx> > >>> --- > >>> 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'" Thanks for advice, will be fixed. > > 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; > >>> > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
