-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, November 7, 2018 12:15 AM To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com Cc: Feng, Shaohe <shaohe.feng@intel.com>; bing.niu@intel.com; Ding, Jian- feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@intel.com> Subject: Re: [PATCHv7 10/18] conf: Remove virDomainResctrlAppend and introduce virDomainResctrlNew
On 11/6/18 4:51 AM, Huaqiang,Wang wrote:
On 2018年11月06日 01:26, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Introduced virDomainResctrlNew to do the most part of virDomainResctrlAppend and move the operation of appending resctrl to @def->resctrls out of function.
Rather than rely on virDomainResctrlAppend to perform the allocation, move the onus to the caller and make use of virBitmapNewCopy for @vcpus and virObjectRef for @alloc, thus removing the need to set each to NULL after the call.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 60 +++++++++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 25 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e8e0adc..39bd396 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18920,26 +18920,22 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt, } -static int -virDomainResctrlAppend(virDomainDefPtr def, - xmlNodePtr node, - virResctrlAllocPtr alloc, - virBitmapPtr vcpus, - unsigned int flags) +static virDomainResctrlDefPtr +virDomainResctrlNew(xmlNodePtr node, + virResctrlAllocPtr *alloc, + virBitmapPtr *vcpus, Because we're not "stealing" @*alloc and/or @*vcpus, they do not need to be passed by reference. I can change these. There's some minor merge impact in later patches too, but no big deal.
Agree. Please help make change.
+ unsigned int flags) { char *vcpus_str = NULL; char *alloc_id = NULL; - virDomainResctrlDefPtr tmp_resctrl = NULL; - int ret = -1; - - if (VIR_ALLOC(tmp_resctrl) < 0) - goto cleanup; + virDomainResctrlDefPtr resctrl = NULL; + virDomainResctrlDefPtr ret = NULL; /* We need to format it back because we need to be consistent in the naming * even when users specify some "sub-optimal" string there. */ - vcpus_str = virBitmapFormat(vcpus); + vcpus_str = virBitmapFormat(*vcpus); if (!vcpus_str) - goto cleanup; + return NULL; if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) alloc_id = virXMLPropString(node, "id"); @@ -18954,18 +18950,23 @@ virDomainResctrlAppend(virDomainDefPtr def, goto cleanup; }
/* NB: Callers assume new @alloc, need to fill in ID now */
Not that it would prevent someone in the future from passing an @alloc w/ ->id already filled in and overwriting, but at least for now it's not the case.
Yes, it might happen. If @alloc->id is specified through XML and is not following the naming convention virAsprintf(&alloc_id, "vcpus_%s", vcpus_str)
If you think it is necessary we might need to through a warning for this case.
Let's see - virDomainResctrlNew has two callers:
1. virDomainCachetuneDefParse
In this case, we "know" we have a new/empty @alloc because if virDomainResctrlVcpuMatch found one, then there'd be a failure.
The virDomainCachetuneDefParseCache calls don't seem to fill in alloc->id, but virDomainResctrlNew will for the first time.
2. virDomainMemorytuneDefParse
The virDomainResctrlVcpuMatch may find a preexisting @alloc, but @new_alloc is set to true. The virDomainMemorytuneDefParseMemory won't fill alloc->id. Then only if @new_alloc do we call virDomainResctrlNew
So I think both are safe "for now".
Yes. Agree. Thanks for the analysis.
If you want I could modify the virResctrlAllocSetID code to :
if (alloc->id) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Attempt to overwrite alloc->id='%s' with id='%s'"), alloc->id, id); return -1; }
Let me know.
virResctrlMonitorSetID should also do similar patch, right? Then the body of two functions, virRresctrlAllocSetID and virResctrlMonitorSetID, are very similar. I will introduce two patches, the first patch will refactor virRresctrlAllocSetID and the second patch will reuse the refactor for virResctrlMonitorSetID. I know you have a solution solving this, my code is just for your reference.
John
Thanks for review. Huaqiang
With the changes (that I can make),
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
Thanks for review. Huaqiang
[...]