[libvirt] [PATCHv5 04/19] util: Add interface for adding PID to monitor

[Wang, Huaqiang]

Hi John,

Last reply is for " [PATCHv5 05/19] util: Refactor code for determining allocation path".

Please ignore this.
Sorry for inconvenience. 

BR
Huaqiang


> -----Original Message-----
> From: Wang, Huaqiang
> Sent: Wednesday, October 10, 2018 9:48 PM
> To: 'John Ferlan' <jferlan at redhat.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 04/19] util: Add interface for adding PID to
> monitor
> 
> 
> 
> > -----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;
>      }
> 
> 
> >
> > > +        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;
> > >