> -----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;
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list