On 10/10/18 9:56 AM, Wang, Huaqiang wrote: > >> -----Original Message----- >> From: John Ferlan [mailto:jferlan@xxxxxxxxxx] >> Sent: Wednesday, October 10, 2018 7:16 AM >> To: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx >> Cc: Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing <bing.niu@xxxxxxxxx>; >> Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui <rui.zang@xxxxxxxxx> >> Subject: Re: [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@xxxxxxxxx> >>> --- >>> 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 >> >>> + >>> + 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__ */ >>> > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
