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,