> -----Original Message----- > From: John Ferlan [mailto:jferlan@xxxxxxxxxx] > Sent: Wednesday, November 7, 2018 12:15 AM > To: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx > Cc: Feng, Shaohe <shaohe.feng@xxxxxxxxx>; bing.niu@xxxxxxxxx; Ding, Jian- > feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui <rui.zang@xxxxxxxxx> > 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@xxxxxxxxx> > >>> --- > >>> 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@xxxxxxxxxx> > >> > >> John > > > > Thanks for review. > > Huaqiang > > > >> > >> [...] > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
