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

2018-10-12 Thread Wang, Huaqiang 



> -Original Message-
> From: John Ferlan [mailto:jfer...@redhat.com]
> Sent: Thursday, October 11, 2018 5:43 AM
> To: Wang, Huaqiang ; libvir-list@redhat.com
> Cc: Feng, Shaohe ; Niu, Bing ;
> Ding, Jian-feng ; Zang, Rui 
> 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:jfer...@redhat.com]
> >> Sent: Wednesday, October 10, 2018 7:16 AM
> >> To: Wang, Huaqiang ; libvir-list@redhat.com
> >> Cc: Feng, Shaohe ; Niu, Bing
> >> ; Ding, Jian-feng ;
> >> Zang, Rui 
> >> 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 
> >>> ---
> >>>  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(, "%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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-10-10 Thread John Ferlan 



On 10/10/18 9:56 AM, Wang, Huaqiang wrote:
> 
>> -Original Message-
>> From: John Ferlan [mailto:jfer...@redhat.com]
>> Sent: Wednesday, October 10, 2018 7:16 AM
>> To: Wang, Huaqiang ; libvir-list@redhat.com
>> Cc: Feng, Shaohe ; Niu, Bing ;
>> Ding, Jian-feng ; Zang, Rui 
>> 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 
>>> ---
>>>  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(, "%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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-10-10 Thread Wang, Huaqiang 


> -Original Message-
> From: John Ferlan [mailto:jfer...@redhat.com]
> Sent: Wednesday, October 10, 2018 7:16 AM
> To: Wang, Huaqiang ; libvir-list@redhat.com
> Cc: Feng, Shaohe ; Niu, Bing ;
> Ding, Jian-feng ; Zang, Rui 
> 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 
> > ---
> >  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(, "%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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-10-09 Thread John Ferlan 



On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> Add interface for resctrl monitor to determine the path.
> 
> Signed-off-by: Wang Huaqiang 
> ---
>  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

> +char *parentpath = NULL;

VIR_AUTOFREE(char *) parentpath = NULL;

(thus the VIR_FREE later isn't necessary)

> +
> +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?

> +
> +if (monitor->alloc)
> +alloc_path = monitor->alloc->path;
> +else
> +alloc_path = (char *)SYSFS_RESCTRL_PATH;

s/(char *)//

> +
> +if (virAsprintf(, "%s/mon_groups", alloc_path) < 0)
> +return -1;
> +
> +monitor->path = virResctrlDeterminePath(parentpath, machinename,
> +monitor->id);
> +
> +VIR_FREE(parentpath);

see above...

John

> +
> +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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-10-09 Thread Wang Huaqiang 
Add interface for resctrl monitor to determine the path.

Signed-off-by: Wang Huaqiang 
---
 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;
+char *parentpath = NULL;
+
+if (!monitor) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Invalid resctrl monitor"));
+return -1;
+}
+
+if (monitor->alloc)
+alloc_path = monitor->alloc->path;
+else
+alloc_path = (char *)SYSFS_RESCTRL_PATH;
+
+if (virAsprintf(, "%s/mon_groups", alloc_path) < 0)
+return -1;
+
+monitor->path = virResctrlDeterminePath(parentpath, machinename,
+monitor->id);
+
+VIR_FREE(parentpath);
+
+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__ */
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list