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