[libvirt] [PATCHv7 05/18] util: Refactor code for adding PID to the resource group

[Wang, Huaqiang]

> -----Original Message-----
> From: John Ferlan [mailto:jferlan at redhat.com]
> Sent: Tuesday, November 6, 2018 10:41 PM
> To: Wang, Huaqiang <huaqiang.wang at intel.com>; libvir-list at redhat.com
> Cc: Feng, Shaohe <shaohe.feng at intel.com>; bing.niu at intel.com; Ding, Jian-
> feng <jian-feng.ding at intel.com>; Zang, Rui <rui.zang at intel.com>
> Subject: Re: [PATCHv7 05/18] util: Refactor code for adding PID to the resource
> group
> 
> 
> 
> On 11/6/18 3:41 AM, Huaqiang,Wang wrote:
> >
> >
> > On 2018年11月05日 23:03, John Ferlan wrote:
> >>
> >> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> >>> The code of adding PID to the allocation could be reused, refactor
> >>> it for later reuse.
> >>>
> >>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> >>> ---
> >>>   src/util/virresctrl.c | 30 +++++++++++++++++++-----------
> >>>   1 file changed, 19 insertions(+), 11 deletions(-)
> >>>
> >> Reviewed-by: John Ferlan <jferlan at redhat.com>
> >>
> >> John
> >
> > Thanks for the review.
> > Huaqiang
> >
> 
> While working through patch1 adjustments, I noted an extra space in a
> comment, so I fixed it:
> 
>     /* If the allocation is empty, then it is impossible to add a PID to
>      * allocation  due to lacking of its 'tasks' file. Just return */
>                  ^^
> Of course that resulted in a merge conflict in this patch where I (now) note you
> changed the comment to:
> 
>     /* If allocation is empty, then no resctrl directory and the 'tasks'
> file
>      * exists, just return */
> 
> I'm going to restore the original comment; however, it made me stop and think
> about future patch14 which used the *tasks (and a new local *pid
> list) - perhaps you need to rethink the changes... Even patch1...
> 
> What's the use of altering the *tasks file at all then?  Fortunately it seems it's not
> really used for much more than logging the tids that are running the vcpu.
> 
> I'll hold off on pushing anything...  So far there's no real impact, but you may
> decide that you need to 'design in' a way to handle this default/system
> path/issue.
> 

I'll send out my v8 patches today, and will be covered in my new series, please make a review.

Thanks for review.

> John