On 08/27/2018 07:23 AM, Wang Huaqiang wrote:
> Changed the interface from
> virDomainResctrlAppend(virDomainDefPtr def,
> xmlNodePtr node,
> virResctrlAllocPtr alloc,
> virBitmapPtr vcpus,
> unsigned int flags);
> to
> virDomainResctrlAppend(virDomainDefPtr def,
> xmlNodePtr node,
> virDomainResctrlDefPtr resctrl,
> unsigned int flags);
>
> Changes will let virDomainRestrlAppend pass through more information
> with virDomainResctrlDefPtr, such as monitoring groups associated
> with the allocation.
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang@xxxxxxxxx>
> ---
> src/conf/domain_conf.c | 48 ++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 34 insertions(+), 14 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index bde9fef..9a65655 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -19247,17 +19247,21 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
> static int
> virDomainResctrlAppend(virDomainDefPtr def,
> xmlNodePtr node,
> - virResctrlAllocPtr alloc,
> - virBitmapPtr vcpus,
> + virDomainResctrlDefPtr resctrl,
> unsigned int flags)
> {
> char *vcpus_str = NULL;
> char *alloc_id = NULL;
> - virDomainResctrlDefPtr tmp_resctrl = NULL;
> + virResctrlAllocPtr alloc = NULL;
> + virBitmapPtr vcpus = NULL;
No need for locals here - just change to resctrl->{alloc|vcpus}
> +
> int ret = -1;
>
> - if (VIR_ALLOC(tmp_resctrl) < 0)
> - goto cleanup;
> + if (!resctrl)
> + return -1;
Again, here we have a programming error without an error message which
results in a generic libvirt an error occurred. Either create a specific
error message or "assume" that your caller has done the right thing.
> +
> + alloc = virObjectRef(resctrl->alloc);
Yikes, how many Ref's are we taking on this? [1]
I don't think this is necessary since we Unref later and currently both
callers do the Ref
> + vcpus = resctrl->vcpus;
>
> /* We need to format it back because we need to be consistent in the naming
> * even when users specify some "sub-optimal" string there. */
> @@ -19281,15 +19285,12 @@ virDomainResctrlAppend(virDomainDefPtr def,
> if (virResctrlAllocSetID(alloc, alloc_id) < 0)
> goto cleanup;
>
> - tmp_resctrl->vcpus = vcpus;
> - tmp_resctrl->alloc = alloc;
> -
> - if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, tmp_resctrl) < 0)
> + if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0)
> goto cleanup;
>
> ret = 0;
> cleanup:
> - virDomainResctrlDefFree(tmp_resctrl);
> + virObjectUnref(alloc);
> VIR_FREE(alloc_id);
> VIR_FREE(vcpus_str);
> return ret;
> @@ -19306,6 +19307,8 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
> xmlNodePtr *nodes = NULL;
> virBitmapPtr vcpus = NULL;
> virResctrlAllocPtr alloc = NULL;
> + virDomainResctrlDefPtr tmp_resctrl = NULL;
> +
> ssize_t i = 0;
> int n;
> int ret = -1;
> @@ -19349,15 +19352,24 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
> goto cleanup;
> }
>
> - if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0)
> + if (VIR_ALLOC(tmp_resctrl) < 0)
> goto cleanup;
> - vcpus = NULL;
> +
> + tmp_resctrl->vcpus = vcpus;
> + tmp_resctrl->alloc = virObjectRef(alloc);
[1] Seems the called function also takes a Ref?
> +
> + if (virDomainResctrlAppend(def, node, tmp_resctrl, flags) < 0)
> + goto cleanup;
> +
> alloc = NULL;
> + vcpus = NULL;
> + tmp_resctrl = NULL;
This sequence is quite familiar with [2] ...
>
> ret = 0;
> cleanup:
> ctxt->node = oldnode;
> virObjectUnref(alloc);
> + VIR_FREE(tmp_resctrl);
> virBitmapFree(vcpus);
> VIR_FREE(nodes);
> return ret;
> @@ -19514,6 +19526,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
> xmlNodePtr *nodes = NULL;
> virBitmapPtr vcpus = NULL;
> virResctrlAllocPtr alloc = NULL;
> + virDomainResctrlDefPtr tmp_resctrl = NULL;
> ssize_t i = 0;
> int n;
> int ret = -1;
> @@ -19560,17 +19573,24 @@ 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)
> + if (VIR_ALLOC(tmp_resctrl) < 0)
> + goto cleanup;
> +
> + tmp_resctrl->alloc = virObjectRef(alloc);
[1] Seems the called function also takes a Ref?
> + tmp_resctrl->vcpus = vcpus;
> + if (virDomainResctrlAppend(def, node, tmp_resctrl, flags) < 0)
> goto cleanup;
> vcpus = NULL;
> alloc = NULL;
> + tmp_resctrl = NULL;
[2] ... this sequence
It seems to me you could create helper :
virDomainResctrlCreate(def, node, alloc, vcpus, flags)
which could :
if (VIR_ALLOC(resctrl) < 0)
return -1;
resctrl->alloc = alloc;
resctrl->vcpus = vcpus;
if (virDomainResctrlAppend(def, node, resctrl, flags) < 0) {
VIR_FREE(resctrl);
return -1;
}
virObjectRef(alloc);
return 0;
with the current callers just changing from Append to Create keeping
their alloc = NULL and vcpus = NULL on success.
John
> }
>
> ret = 0;
> cleanup:
> ctxt->node = oldnode;
> - virObjectUnref(alloc);
> virBitmapFree(vcpus);
> + virObjectUnref(alloc);
> + VIR_FREE(tmp_resctrl);
> VIR_FREE(nodes);
> return ret;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
