Re: [libvirt] [PATCHv5 04/19] util: Add interface for adding PID to monitor
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(&path, "%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(&alloc->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
> -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(&path, "%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(&alloc->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
> -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
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
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