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

[Wang, Huaqiang]

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