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

Wang, Huaqiang huaqiang.wang at intel.com
Fri Oct 12 10:31:18 UTC 2018



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




More information about the libvir-list mailing list