I see your point, yes, you are right, a leak might occur and I'll fix it
in the cleanup phase. Just to be correct, it is not related to the
error-check you pointed out. It is because up until now, numatune was
only a local duplicate pointer to an allocated memory which was
referenced by *numatunePtr (passed as argument). And this was the core
of the bug, working directly with a pointer passed to the function. If
any problem occures, that pointer should stay untouched, that's why I
slightly changed the allocation, thus unfortunately creating a
possibility for the leak. So I'm putting another 'if(create)' clause
inside the cleanup phase in PATCHv2.
Regards,
Erik
On 08/20/2014 02:50 PM, Ján Tomko wrote:
On 08/20/2014 12:49 PM, Erik Skultety wrote:
When trying to set numatune mode directly using virsh numatune command,
correct error is raised, however numatune structure was not deallocated,
thus resulting in creating an empty numatune element in the guest XML,
if none was present before. Running the same command aftewards results
in a successful change with broken XML structure. Patch fixes the
deallocation problem as well as checking for invalid attribute
combination VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO + a nonempty nodeset.
Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1129998
---
src/conf/numatune_conf.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c
index 48d1d04..45bf0cb 100644
--- a/src/conf/numatune_conf.c
+++ b/src/conf/numatune_conf.c
@@ -439,7 +439,7 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr,
{
bool create = !*numatunePtr; /* Whether we are creating new struct */
int ret = -1;
- virDomainNumatunePtr numatune = NULL;
+ virDomainNumatunePtr numatune = *numatunePtr;
/* No need to do anything in this case */
if (mode == -1 && placement == -1 && !nodeset)
@@ -461,9 +461,15 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr,
goto cleanup;
}
- if (create && VIR_ALLOC(*numatunePtr) < 0)
+ if (placement_static && !nodeset) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("nodeset for NUMA memory tuning must be set "
+ "if 'placement' is 'static'"));
+ goto cleanup;
You moved this error above the allocation, but there is another 'goto cleanup'
possible after the successful allocation of 'numatune':
if (nodeset) {
virBitmapFree(numatune->memory.nodeset);
numatune->memory.nodeset = virBitmapNewCopy(nodeset);
if (!numatune->memory.nodeset)
goto cleanup;
if (placement == -1)
placement = VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC;
}
If virBitmapNewCopy fails and numatune was allocated by this function, it's
leaked.
Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
