Re: [libvirt] [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(a)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". 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.
John
>
> With the changes (that I can make),
>
> Reviewed-by: John Ferlan <jferlan(a)redhat.com>
>
> John
Thanks for review.
Huaqiang
>
> [...]