Re: [libvirt] [PATCHv5 04/19] util: Add interface for adding PID to monitor

2018-10-10 Thread Wang, Huaqiang 
Hi John,

Last reply is for " [PATCHv5 05/19] util: Refactor code for determining 
allocation path".

Please ignore this.
Sorry for inconvenience. 

BR
Huaqiang


> -Original Message-
> From: Wang, Huaqiang
> Sent: Wednesday, October 10, 2018 9:48 PM
> To: 'John Ferlan' ; libvir-list@redhat.com
> Cc: Feng, Shaohe ; Niu, Bing ;
> Ding, Jian-feng ; Zang, Rui 
> Subject: RE: [libvirt] [PATCHv5 04/19] util: Add interface for adding PID to
> monitor
> 
> 
> 
> > -Original Message-
> > From: John Ferlan [mailto:jfer...@redhat.com]
> > Sent: Wednesday, October 10, 2018 7:09 AM
> > To: Wang, Huaqiang ; libvir-list@redhat.com
> > Cc: Feng, Shaohe ; Niu, Bing
> > ; Ding, Jian-feng ;
> > Zang, Rui 
> > Subject: Re: [libvirt] [PATCHv5 05/19] util: Refactor code for
> > determining allocation path
> >
> >
> >
> > On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> > > The code for determining resctrl allocation path could be reused for
> > > monitor. Refactor it for reusing.
> > >
> > > Signed-off-by: Wang Huaqiang 
> > > ---
> > >  src/util/virresctrl.c | 33 +++--
> > >  1 file changed, 27 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index
> > > c9a79f7..03001cc 100644
> > > --- a/src/util/virresctrl.c
> > > +++ b/src/util/virresctrl.c
> > > @@ -2267,6 +2267,26 @@ virResctrlAllocAssign(virResctrlInfoPtr
> > > resctrl,  }
> > >
> > >
> > > +static char *
> > > +virResctrlDeterminePath(const char *pathparent,
> >
> > s/pathparent/parentpath
> >
> 
> OK.
> 
> > > +const char *prefix,
> > > +const char *id) {
> > > +char *path = NULL;
> > > +
> > > +if (!id) {
> > > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > +   _("Resctrl resource ID must be set before
> > > + creation"));
> >
> > s/Resctrl resource ID/'%s' resctrl ID/
> >
> > where %s is @parentpath
> >
> > Working in the @parentpath, then we'd know which it was Alloc or
> > Monitor
> 
> How about changes like these?
> 
>  if (!id) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("Resctrl resource ID must be set before creation"));
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Resctrl ID must be set before determining resctrl "
> + "path under '%s'"),
> +   parentpath);
>  return NULL;
>  }
> 
> 
> >
> > > +return NULL;
> > > +}
> > > +
> > > +if (virAsprintf(, "%s/%s-%s", pathparent, prefix, id) < 0)
> >
> > parentpath
> 
> Will be renamed.
> 
> >
> > > +return NULL;
> > > +
> > > +return path;
> > > +}
> > > +
> > > +
> > >  int
> > >  virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> > >   const char *machinename) @@ -2274,15
> > > +2294,16 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> > >  if (!alloc)
> > >  return 0;
> > >
> > > -if (!alloc->id) {
> > > -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > -   _("Resctrl Allocation ID must be set before 
> > > creation"));
> > > +if (alloc->path) {
> > > +virReportError(VIR_ERR_INVALID_ARG, "%s",
> > > +   _("Resctrl group path is expected to be
> > > + NULL"));
> >
> > Resctrl alloc group path is already set '%s'
> >
> > I think this is an internal error not an invalid arg, too
> 
> Will be changed.
> 
>  if (alloc->path) {
> -virReportError(VIR_ERR_INVALID_ARG, "%s",
> -   _("Resctrl group path is expected to be NULL"));
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Resctrl allocation path is already set to '%s'"),
> +   alloc->path);
>  return -1;
>  }
> 
> >
> > John
> >
> 
> 
> Thanks for review.
> Huaqiang
> 
> > >  return -1;
> > >  }
> > >
> > > -if (!alloc->path &&
> > > -virAsprintf(>path, "%s/%s-%s",
> > > -SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
> > > +alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH,
> > > +  machinename,
> > > +  alloc->id);
> > > +if (!alloc->path)
> > >  return -1;
> > >
> > >  return 0;
> > >


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

Re: [libvirt] [PATCHv5 04/19] util: Add interface for adding PID to monitor

2018-10-10 Thread Wang, Huaqiang 



> -Original Message-
> From: John Ferlan [mailto:jfer...@redhat.com]
> Sent: Wednesday, October 10, 2018 7:09 AM
> To: Wang, Huaqiang ; libvir-list@redhat.com
> Cc: Feng, Shaohe ; Niu, Bing ;
> Ding, Jian-feng ; Zang, Rui 
> Subject: Re: [libvirt] [PATCHv5 05/19] util: Refactor code for determining
> allocation path
> 
> 
> 
> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> > The code for determining resctrl allocation path could be reused for
> > monitor. Refactor it for reusing.
> >
> > Signed-off-by: Wang Huaqiang 
> > ---
> >  src/util/virresctrl.c | 33 +++--
> >  1 file changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index
> > c9a79f7..03001cc 100644
> > --- a/src/util/virresctrl.c
> > +++ b/src/util/virresctrl.c
> > @@ -2267,6 +2267,26 @@ virResctrlAllocAssign(virResctrlInfoPtr
> > resctrl,  }
> >
> >
> > +static char *
> > +virResctrlDeterminePath(const char *pathparent,
> 
> s/pathparent/parentpath
> 

OK.

> > +const char *prefix,
> > +const char *id) {
> > +char *path = NULL;
> > +
> > +if (!id) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +   _("Resctrl resource ID must be set before
> > + creation"));
> 
> s/Resctrl resource ID/'%s' resctrl ID/
> 
> where %s is @parentpath
> 
> Working in the @parentpath, then we'd know which it was Alloc or Monitor

How about changes like these?

 if (!id) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Resctrl resource ID must be set before creation"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Resctrl ID must be set before determining resctrl "
+ "path under '%s'"),
+   parentpath);
 return NULL;
 }


> 
> > +return NULL;
> > +}
> > +
> > +if (virAsprintf(, "%s/%s-%s", pathparent, prefix, id) < 0)
> 
> parentpath

Will be renamed.

> 
> > +return NULL;
> > +
> > +return path;
> > +}
> > +
> > +
> >  int
> >  virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> >   const char *machinename) @@ -2274,15
> > +2294,16 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> >  if (!alloc)
> >  return 0;
> >
> > -if (!alloc->id) {
> > -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > -   _("Resctrl Allocation ID must be set before 
> > creation"));
> > +if (alloc->path) {
> > +virReportError(VIR_ERR_INVALID_ARG, "%s",
> > +   _("Resctrl group path is expected to be
> > + NULL"));
> 
> Resctrl alloc group path is already set '%s'
> 
> I think this is an internal error not an invalid arg, too

Will be changed.

 if (alloc->path) {
-virReportError(VIR_ERR_INVALID_ARG, "%s",
-   _("Resctrl group path is expected to be NULL"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Resctrl allocation path is already set to '%s'"),
+   alloc->path);
 return -1;
 }

> 
> John
> 


Thanks for review.
Huaqiang

> >  return -1;
> >  }
> >
> > -if (!alloc->path &&
> > -virAsprintf(>path, "%s/%s-%s",
> > -SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
> > +alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH,
> > +  machinename,
> > +  alloc->id);
> > +if (!alloc->path)
> >  return -1;
> >
> >  return 0;
> >


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

Re: [libvirt] [PATCHv5 04/19] util: Add interface for adding PID to monitor

2018-10-10 Thread Wang, Huaqiang 



> -Original Message-
> From: John Ferlan [mailto:jfer...@redhat.com]
> Sent: Wednesday, October 10, 2018 7:09 AM
> To: Wang, Huaqiang ; libvir-list@redhat.com
> Cc: Feng, Shaohe ; Niu, Bing ;
> Ding, Jian-feng ; Zang, Rui 
> Subject: Re: [libvirt] [PATCHv5 04/19] util: Add interface for adding PID to
> monitor
> 
> 
> 
> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> > Add interface for adding task PID to monitor.
> 
> to the monitor

Will be changed.

> 
> >
> > Signed-off-by: Wang Huaqiang 
> > ---
> >  src/libvirt_private.syms | 1 +
> >  src/util/virresctrl.c| 8 
> >  src/util/virresctrl.h| 4 
> >  3 files changed, 13 insertions(+)
> 
> Kind of an odd order considering @path gets introduced later, but it's not 
> that
> big a deal.
> 
> Reviewed-by: John Ferlan 
> 
> John

Thanks for review.
Huaqiang


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

Re: [libvirt] [PATCHv5 04/19] util: Add interface for adding PID to monitor

2018-10-09 Thread John Ferlan 



On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> Add interface for adding task PID to monitor.

to the monitor

> 
> Signed-off-by: Wang Huaqiang 
> ---
>  src/libvirt_private.syms | 1 +
>  src/util/virresctrl.c| 8 
>  src/util/virresctrl.h| 4 
>  3 files changed, 13 insertions(+)

Kind of an odd order considering @path gets introduced later, but it's
not that big a deal.

Reviewed-by: John Ferlan 

John

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

[libvirt] [PATCHv5 04/19] util: Add interface for adding PID to monitor

2018-10-09 Thread Wang Huaqiang 
Add interface for adding task PID to monitor.

Signed-off-by: Wang Huaqiang 
---
 src/libvirt_private.syms | 1 +
 src/util/virresctrl.c| 8 
 src/util/virresctrl.h| 4 
 3 files changed, 13 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d2573c5..4a52a86 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2680,6 +2680,7 @@ virResctrlInfoGetCache;
 virResctrlInfoGetMonitorPrefix;
 virResctrlInfoMonFree;
 virResctrlInfoNew;
+virResctrlMonitorAddPID;
 virResctrlMonitorNew;
 
 
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 41c7e51..c9a79f7 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2436,3 +2436,11 @@ virResctrlMonitorNew(void)
 
 return virObjectNew(virResctrlMonitorClass);
 }
+
+
+int
+virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
+pid_t pid)
+{
+return virResctrlAddPID(monitor->path, pid);
+}
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index f59a9aa..cb9bfae 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -192,4 +192,8 @@ typedef virResctrlMonitor *virResctrlMonitorPtr;
 
 virResctrlMonitorPtr
 virResctrlMonitorNew(void);
+
+int
+virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
+pid_t pid);
 #endif /*  __VIR_RESCTRL_H__ */
-- 
2.7.4

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