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