[libvirt] [PATCHv7 01/18] docs, util: Refactor schemas and virresctrl to support optional cache

[Huaqiang,Wang]

On 2018年11月06日 16:18, Huaqiang,Wang wrote:
>
>
> On 2018年11月05日 23:01, John Ferlan wrote:
>>
>> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
>>> Refactor schemas and virresctrl to support optional <cache> element
>>> in <cachetune>.
>>>
>>> Later, the monitor entry will be introduced and to be placed
>>> under <cachetune>. Either cache entry or monitor entry is
>>> an optional element of <cachetune>.
>>>
>>> An cachetune has no <cache> element is taking the default resource
>>> allocating policy defined in '/sys/fs/resctrl/schemata'.
>>>
>>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
>>> ---
>>>   docs/formatdomain.html.in     |  4 ++--
>>>   docs/schemas/domaincommon.rng |  4 ++--
>>>   src/util/virresctrl.c         | 28 ++++++++++++++++++++++++++++
>>>   3 files changed, 32 insertions(+), 4 deletions(-)
>>>
>> [...]
>>
>>> +    /* If the allocation is empty, then the path will be 
>>> SYSFS_RESCTRL_PATH */
>>> +    if (virResctrlAllocIsEmpty(alloc)) {
>>> +        if (!alloc->path &&
>>> +            VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0)
>>> +            return -1;
>>> +
>>> +        return 0;
>>> +    }
>>> +
>> Because of ...
>>
>>>       if (!alloc->path &&
>>>           virAsprintf(&alloc->path, "%s/%s-%s",
>>>                       SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
>> [...]
>>
>>> @@ -2334,6 +2358,10 @@ virResctrlAllocRemove(virResctrlAllocPtr alloc)
>>>   {
>>>       int ret = 0;
>>>   +    /* No directory have ever been created. Just return */
>>> +    if (virResctrlAllocIsEmpty(alloc))
>>> +        return 0;
>> ... the change to virResctrlAllocDeterminePath to fill in alloc->path
>> when virResctrlAllocIsEmpty to be a default path, this should be:
>>
>>      if (STREQ_NULLABLE(alloc->path, SYSFS_RESCTRL_PATH))
>>         return 0;
>>
>> or moved after the next check and the _NULLABLE removed.
>>
>> Whether the AllocIsEmpty is true or not shouldn't be the bearing on
>> whether the directory created because of that
>
> Agree with the changes.
> "No need to create a directory that has already been created by system."
> (SYSFS_RESCTRL_PATH is the created directory).

Should be "no need to destroy a system created directory, the 
SYSFS_RESCTRL_PATH.
Directory SYSFS_RESCTRL_PATH is governed by system."

It might also be reasonable to make similar changes for 
virResctrlAllocCreate,
because no need to create an already created directory, Right? And 
*alloc->path must
be properly assigned, by virResctrlAllocDeterminePath, before a call of 
virResctrlAllocCreate.

>
>>> +
>>>       if (!alloc->path)
>>>           return 0;
>>>
>> I can adjust for you, let me know; otherwise, things are fine for the
>>
>> Reviewed-by: John Ferlan <jferlan at redhat.com>
>>
>> John
>
> Thanks for the review.
> Huaqiang
>