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

[John Ferlan]

On 11/6/18 3:30 AM, Huaqiang,Wang wrote:
> 
> 
> 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."
> 

I'll add the comment:

+    /* Do not destroy if path is the system/default path for the
allocation */

> 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.
> 

True - I'll adjust the check there too to be:

+    /* If using the system/default path for the allocation, then we're
done */
+    if (STREQ(alloc->path, SYSFS_RESCTRL_PATH))
+        return 0;
+

Tks -

John

>>
>>>> +
>>>>       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
>>
>