[libvirt] [PATCH 11/28] qemu: eliminate memory leaks when converting NetDefs to type='ethernet'

[Laine Stump]

On 06/23/2016 05:59 PM, John Ferlan wrote:
>
> On 06/22/2016 01:37 PM, Laine Stump wrote:
>> in qemuConnectDomainXMLToNative. This function was only accounting for
>> about 1/10 of all the allocated items in the NetDef prior to memseting
>> it to all 0's. On top of that, it was going to great pains to learn
>> the name of the bridge device, but then never doing anything useful
>> with it (just putting it into data.ethernet.dev, which is *never* used
>> when building a qemu commandline). (I think this again all started off
>> as code with good intentions, but it was never completed, and instead
>> was just Frankensteinically cargo-culted into the odd mish mash we
>> have today).
>>
>> The resulting code is much simpler, produces exactly the same output,
>> and doesn't leak memory.
>> ---
>>   src/qemu/qemu_driver.c | 56 ++++++--------------------------------------------
>>   1 file changed, 6 insertions(+), 50 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 517d0b8..4a8cb7a 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -6987,62 +6987,18 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn,
>>           unsigned int bootIndex = net->info.bootIndex;
>>           char *model = net->model;
>>           virMacAddr mac = net->mac;
>> +        char *script = net->script;
> Based on 3 spots below where net->script was set to NULL, should this
> only be set when "(net->type == VIR_DOMAIN_NET_TYPE_BRIDGE)" ?

Actually I think that since it's in the common part of the object (and 
not in the bridge-specific part) that it should just *always* be saved 
and re-set. I'll do that before I push.

>
>
>>   
>> -        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
>> -            int actualType = virDomainNetGetActualType(net);
>> -            const char *brname;
>> -
>> -            VIR_FREE(net->data.network.name);
>> -            VIR_FREE(net->data.network.portgroup);
>> -            if ((actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) &&
>> -                (brname = virDomainNetGetActualBridgeName(net))) {
>> -
>> -                char *brnamecopy;
>> -
>> -                if (VIR_STRDUP(brnamecopy, brname) < 0)
>> -                    goto cleanup;
>> -
>> -                virDomainActualNetDefFree(net->data.network.actual);
>> -
>> -                memset(net, 0, sizeof(*net));
>> -
>> -                net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
>> -                net->script = NULL;
> ^^^
>
>> -                net->data.ethernet.dev = brnamecopy;
>> -            } else {
>> -                /* actualType is either NETWORK or DIRECT. In either
>> -                 * case, the best we can do is NULL everything out.
>> -                 */
>> -                virDomainActualNetDefFree(net->data.network.actual);
>> -                memset(net, 0, sizeof(*net));
>> -
>> -                net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
>> -                net->script = NULL;
> ^^^
>
>> -                net->data.ethernet.dev = NULL;
>> -            }
>> -        } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
>> -            VIR_FREE(net->data.direct.linkdev);
>> +        net->model = NULL;
>> +        net->script = NULL;
>>   
>> -            memset(net, 0, sizeof(*net));
>> -
>> -            net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
>> -            net->script = NULL;
> ^^^
>
> ACK - whichever way you go with that 'script' setting. I figure you know
> the best answer to my question
>
>
> John
>> -            net->data.ethernet.dev = NULL;
>> -        } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>> -            char *script = net->script;
>> -            char *brname = net->data.bridge.brname;
>> -
>> -            memset(net, 0, sizeof(*net));
>> -
>> -            net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
>> -            net->script = script;
>> -            net->data.ethernet.dev = brname;
>> -        }
>> +        virDomainNetDefClear(net);
>>   
>> -        VIR_FREE(net->virtPortProfile);
>> +        net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
>>           net->info.bootIndex = bootIndex;
>>           net->model = model;
>>           net->mac = mac;
>> +        net->script = script;
>>       }
>>   
>>       if (!(cmd = qemuProcessCreatePretendCmd(conn, driver, vm, NULL,
>>