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

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Tuesday, November 6, 2018 10:41 PM To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; bing.niu(a)intel.com; Ding, Jian- feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; 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(a)intel.com&gt; >>> --- >>>   src/util/virresctrl.c | 30 +++++++++++++++++++----------- >>>   1 file changed, 19 insertions(+), 11 deletions(-) >>> >> Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; >> > > > 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.

John