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". 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@xxxxxxxxxx>
>>
>> John
>
> Thanks for review.
> Huaqiang
>
>>
>> [...]
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
