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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxx>
>>> ---
>>>   src/util/virresctrl.c | 30 +++++++++++++++++++-----------
>>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>>
>> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
>>
>> 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.

John

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux