5:33 a.m.
-----Original Message-----
From: John Ferlan [mailto:jferlan@redhat.com]
Sent: Thursday, October 11, 2018 5:43 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>
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(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>
>> 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>
>>> ---
>>> 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__ */
>>>
>