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

> -----Original Message----- > From: John Ferlan [mailto:jferlan@redhat.com] > Sent: Wednesday, October 10, 2018 7:16 AM > To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com > Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>; > Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; > 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(a)intel.com&gt; >> --- >> 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.

> >> + >> + 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__ */ >>