[libvirt] [PATCHv5 06/19] util: Add monitor interface to determine path

[Wang, Huaqiang]

> -----Original Message-----
> From: John Ferlan [mailto:jferlan at redhat.com]
> Sent: Thursday, October 11, 2018 5:43 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 06/19] util: Add monitor interface to determine
> path
> 
> 
> 
> On 10/10/18 9:56 AM, Wang, Huaqiang wrote:
> >
> >> -----Original Message-----
> >> From: John Ferlan [mailto:jferlan at redhat.com]
> >> Sent: Wednesday, October 10, 2018 7:16 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 06/19] util: Add monitor interface to
> >> determine path
> >>
> >>
> >>
> >> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> >>> Add interface for resctrl monitor to determine the path.
> >>>
> >>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> >>> ---
> >>>  src/libvirt_private.syms |  1 +
> >>>  src/util/virresctrl.c    | 32 ++++++++++++++++++++++++++++++++
> >>>  src/util/virresctrl.h    |  3 +++
> >>>  3 files changed, 36 insertions(+)
> >>>
> >>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> >>> index 4a52a86..e175c8b 100644
> >>> --- a/src/libvirt_private.syms
> >>> +++ b/src/libvirt_private.syms
> >>> @@ -2681,6 +2681,7 @@ virResctrlInfoGetMonitorPrefix;
> >>> virResctrlInfoMonFree;  virResctrlInfoNew;  virResctrlMonitorAddPID;
> >>> +virResctrlMonitorDeterminePath;
> >>>  virResctrlMonitorNew;
> >>>
> >>>
> >>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index
> >>> 03001cc..1a5578e 100644
> >>> --- a/src/util/virresctrl.c
> >>> +++ b/src/util/virresctrl.c
> >>> @@ -2465,3 +2465,35 @@ virResctrlMonitorAddPID(virResctrlMonitorPtr
> >>> monitor,  {
> >>>      return virResctrlAddPID(monitor->path, pid);  }
> >>> +
> >>> +int
> >>> +virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
> >>> +                               const char *machinename) {
> >>> +    char *alloc_path = NULL;
> >>
> >> const char
> >
> > OK, a pointer to const char will be better.
> >
> >>
> >>> +    char *parentpath = NULL;
> >>
> >> VIR_AUTOFREE(char *) parentpath = NULL;
> >>
> >> (thus the VIR_FREE later isn't necessary)
> >
> > I haven't realized the existence of such kind of variable 'decorator'.
> > VIR_AUTOFREE will be used in next update.
> > Thanks.
> >
> 
> Yeah it's "newer" stuff, but it hasn't always gotten into my review cadence so
> sometimes I remember, sometimes I don't. We had a Google summer of code
> student working through those changes, but there's still many more places to
> change.
> 
> John
> 

Thanks!
Huaqiang

> >>
> >>> +
> >>> +    if (!monitor) {
> >>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >>> +                       _("Invalid resctrl monitor"));
> >>> +        return -1;
> >>> +    }
> >>
> >> Shouldn't there be a monitor->path check here like there is in
> >> virResctrlAllocDeterminePath?
> >
> > Since virResctrlAllocDeterminePath has such kind of safety check.
> > Let's add the similar check here.
> > will be added.
> >
> >>
> >>> +
> >>> +    if (monitor->alloc)
> >>> +        alloc_path = monitor->alloc->path;
> >>> +    else
> >>> +        alloc_path = (char *)SYSFS_RESCTRL_PATH;
> >>
> >> s/(char *)//
> >
> > Will be removed.
> >
> >>
> >>> +
> >>> +    if (virAsprintf(&parentpath, "%s/mon_groups", alloc_path) < 0)
> >>> +        return -1;
> >>> +
> >>> +    monitor->path = virResctrlDeterminePath(parentpath, machinename,
> >>> +                                            monitor->id);
> >>> +
> >>> +    VIR_FREE(parentpath);
> >>
> >> see above...
> >
> > Line will be removed.
> >
> >>
> >> John
> >
> > Thanks for review.
> > Huaqiang
> >
> >>
> >>> +
> >>> +    if (!monitor->path)
> >>> +        return -1;
> >>> +
> >>> +    return 0;
> >>> +}
> >>> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index
> >>> cb9bfae..69b6b1d 100644
> >>> --- a/src/util/virresctrl.h
> >>> +++ b/src/util/virresctrl.h
> >>> @@ -196,4 +196,7 @@ virResctrlMonitorNew(void);  int
> >>> virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
> >>>                          pid_t pid);
> >>> +int
> >>> +virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
> >>> +                               const char *machinename);
> >>>  #endif /*  __VIR_RESCTRL_H__ */
> >>>
> >