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

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(a)intel.com&gt; >>> --- >>>   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(a)redhat.com&gt; >> >> John > > Thanks for the review. > Huaqiang >