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

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)" ?

> > - 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, >