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,
+ 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;
}
- if (virResctrlAllocSetID(alloc, alloc_id) < 0)
+ if (virResctrlAllocSetID(*alloc, alloc_id) < 0)
goto cleanup;
- tmp_resctrl->vcpus = vcpus;
- tmp_resctrl->alloc = alloc;
+ if (VIR_ALLOC(resctrl) < 0)
+ goto cleanup;
- if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, tmp_resctrl) < 0)
+ if (!(resctrl->vcpus = virBitmapNewCopy(*vcpus))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to copy 'vcpus'"));
goto cleanup;
+ }
- ret = 0;
+ resctrl->alloc = virObjectRef(*alloc);
+
+ VIR_STEAL_PTR(ret, resctrl);
cleanup:
- virDomainResctrlDefFree(tmp_resctrl);
+ virDomainResctrlDefFree(resctrl);
VIR_FREE(alloc_id);
VIR_FREE(vcpus_str);
return ret;
@@ -18982,6 +18983,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
xmlNodePtr *nodes = NULL;
virBitmapPtr vcpus = NULL;
virResctrlAllocPtr alloc = NULL;
+ virDomainResctrlDefPtr resctrl = NULL;
ssize_t i = 0;
int n;
int ret = -1;
@@ -19025,14 +19027,17 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
goto cleanup;
}
- if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0)
+ resctrl = virDomainResctrlNew(node, &alloc, &vcpus, flags);
+ if (!resctrl)
+ goto cleanup;
+
+ if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0)
goto cleanup;
- vcpus = NULL;
- alloc = NULL;
ret = 0;
cleanup:
ctxt->node = oldnode;
+ virDomainResctrlDefFree(resctrl);
virObjectUnref(alloc);
virBitmapFree(vcpus);
VIR_FREE(nodes);
@@ -19190,6 +19195,8 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
xmlNodePtr *nodes = NULL;
virBitmapPtr vcpus = NULL;
virResctrlAllocPtr alloc = NULL;
+ virDomainResctrlDefPtr resctrl = NULL;
+
ssize_t i = 0;
int n;
int ret = -1;
@@ -19234,15 +19241,18 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
* just update the existing alloc information, which is done in above
* virDomainMemorytuneDefParseMemory */
if (new_alloc) {
- if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0)
+ resctrl = virDomainResctrlNew(node, &alloc, &vcpus, flags);
+ if (!resctrl)
+ goto cleanup;
+
+ if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0)
goto cleanup;
- vcpus = NULL;
- alloc = NULL;
}
ret = 0;
cleanup:
ctxt->node = oldnode;
+ virDomainResctrlDefFree(resctrl);
virObjectUnref(alloc);
virBitmapFree(vcpus);
VIR_FREE(nodes);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
> + 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.
With the changes (that I can make),
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
>
> With the changes (that I can make),
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John
Thanks for review.
Huaqiang
>
> [...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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". 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@redhat.com>
>>
>> John
>
> Thanks for review.
> Huaqiang
>
>>
>> [...]
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
> -----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
> >
> >>
> >> [...]
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.