10:48 a.m.
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(a)intel.com>
---
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;
}