Re: [libvirt] [PATCHv4 1/3] qemu_migration: Add hooks to transport network data during migration

On 22.10.2012 23:30, Laine Stump wrote: > From: Kyle Mestery <kmestery(a)cisco.com&gt; > > Add the ability for the Qemu V3 migration protocol to > include transporting network configuration. A generic > framework is proposed with this patch to allow for the > transfer of opaque data. > > Signed-off-by: Kyle Mestery <kmestery(a)cisco.com&gt; > --- > src/qemu/qemu_migration.c | 238 +++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 236 insertions(+), 2 deletions(-) ACK but see my comments below. > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 487182e..67276f0 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -1,3 +1,4 @@ > + > /* > * qemu_migration.c: QEMU migration handling > * > @@ -70,6 +71,7 @@ enum qemuMigrationCookieFlags { > QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS, > QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE, > QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT, > + QEMU_MIGRATION_COOKIE_FLAG_NETWORK, > > QEMU_MIGRATION_COOKIE_FLAG_LAST > }; > @@ -77,12 +79,13 @@ enum qemuMigrationCookieFlags { > VIR_ENUM_DECL(qemuMigrationCookieFlag); > VIR_ENUM_IMPL(qemuMigrationCookieFlag, > QEMU_MIGRATION_COOKIE_FLAG_LAST, > - "graphics", "lockstate", "persistent"); > + "graphics", "lockstate", "persistent", "network"); > > enum qemuMigrationCookieFeatures { > QEMU_MIGRATION_COOKIE_GRAPHICS = (1 << QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS), > QEMU_MIGRATION_COOKIE_LOCKSTATE = (1 << QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE), > QEMU_MIGRATION_COOKIE_PERSISTENT = (1 << QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT), > + QEMU_MIGRATION_COOKIE_NETWORK = (1 << QEMU_MIGRATION_COOKIE_FLAG_NETWORK), > }; > > typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics; > @@ -95,6 +98,27 @@ struct _qemuMigrationCookieGraphics { > char *tlsSubject; > }; > > +typedef struct _qemuMigrationCookieNetdata qemuMigrationCookieNetdata; > +typedef qemuMigrationCookieNetdata *qemuMigrationCookieNetdataPtr; > +struct _qemuMigrationCookieNetdata { > + int vporttype; /* enum virNetDevVPortProfile */ > + > + /* > + * Array of pointers to saved data. Each VIF will have it's own > + * data to transfer. > + */ > + char *portdata; > +}; > + > +typedef struct _qemuMigrationCookieNetwork qemuMigrationCookieNetwork; > +typedef qemuMigrationCookieNetwork *qemuMigrationCookieNetworkPtr; > +struct _qemuMigrationCookieNetwork { > + /* How many virtual NICs are we saving data for? */ > + int nnets; > + > + qemuMigrationCookieNetdataPtr net; > +}; > + > typedef struct _qemuMigrationCookie qemuMigrationCookie; > typedef qemuMigrationCookie *qemuMigrationCookiePtr; > struct _qemuMigrationCookie { > @@ -120,6 +144,9 @@ struct _qemuMigrationCookie { > > /* If (flags & QEMU_MIGRATION_COOKIE_PERSISTENT) */ > virDomainDefPtr persistent; > + > + /* If (flags & QEMU_MIGRATION_COOKIE_NETWORK) */ > + qemuMigrationCookieNetworkPtr network; > }; > > static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap) > @@ -132,6 +159,23 @@ static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap) > } > > > +static void > +qemuMigrationCookieNetworkFree(qemuMigrationCookieNetworkPtr network) > +{ > + int i; > + > + if (!network) > + return; > + > + if (network->net) { > + for (i = 0; i < network->nnets; i++) > + VIR_FREE(network->net[i].portdata); > + } You could have spared one level of nesting if you'd just only ... [1]

> + VIR_FREE(network->net); > + VIR_FREE(network); > +} > + > + > static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig) > { > if (!mig) > @@ -140,6 +184,10 @@ static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig) > if (mig->flags & QEMU_MIGRATION_COOKIE_GRAPHICS) > qemuMigrationCookieGraphicsFree(mig->graphics); > > + if (mig->flags & QEMU_MIGRATION_COOKIE_NETWORK) { > + qemuMigrationCookieNetworkFree(mig->network); > + } These curly braces aren't required. But it is not against coding style neither. > + > VIR_FREE(mig->localHostname); > VIR_FREE(mig->remoteHostname); > VIR_FREE(mig->name); > @@ -256,6 +304,49 @@ error: > } > > > +static qemuMigrationCookieNetworkPtr > +qemuMigrationCookieNetworkAlloc(struct qemud_driver *driver ATTRIBUTE_UNUSED, > + virDomainDefPtr def) > +{ > + qemuMigrationCookieNetworkPtr mig; > + int i; > + > + if (VIR_ALLOC(mig) < 0) > + goto no_memory; > + > + mig->nnets = def->nnets; [1]: ... set this after the VIR_ALLOC_N below.

> + > + if (VIR_ALLOC_N(mig->net, def->nnets) <0) > + goto no_memory; > + > + for (i = 0; i < def->nnets; i++) { > + virDomainNetDefPtr netptr; > + virNetDevVPortProfilePtr vport; > + > + netptr = def->nets[i]; > + vport = virDomainNetGetActualVirtPortProfile(netptr); > + > + if (vport) { > + mig->net[i].vporttype = vport->virtPortType; > + > + switch (vport->virtPortType) { > + case VIR_NETDEV_VPORT_PROFILE_NONE: > + case VIR_NETDEV_VPORT_PROFILE_8021QBG: > + case VIR_NETDEV_VPORT_PROFILE_8021QBH: > + case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: > + default: > + break; > + } > + } > + } > + return mig; > + > +no_memory: > + virReportOOMError(); > + qemuMigrationCookieNetworkFree(mig); > + return NULL; > +} > + > static qemuMigrationCookiePtr > qemuMigrationCookieNew(virDomainObjPtr dom) > { > @@ -370,6 +461,27 @@ qemuMigrationCookieAddPersistent(qemuMigrationCookiePtr mig, > } > > > +static int > +qemuMigrationCookieAddNetwork(qemuMigrationCookiePtr mig, > + struct qemud_driver *driver, > + virDomainObjPtr dom) > +{ > + if (mig->flags & QEMU_MIGRATION_COOKIE_NETWORK) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Network migration data already present")); > + return -1; > + } > + > + if (dom->def->nnets > 0) { > + mig->network = qemuMigrationCookieNetworkAlloc(driver, dom->def); > + if (!mig->network) > + return -1; > + mig->flags |= QEMU_MIGRATION_COOKIE_NETWORK; > + } > + > + return 0; > +} > + > > static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf, > qemuMigrationCookieGraphicsPtr grap) > @@ -389,6 +501,32 @@ static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf, > } > > > +static void > +qemuMigrationCookieNetworkXMLFormat(virBufferPtr buf, > + qemuMigrationCookieNetworkPtr optr) > +{ > + int i; > + > + virBufferAsprintf(buf, " <network>\n"); > + for (i = 0; i < optr->nnets; i++) { > + /* If optr->net.vporttype[i] is not set, there is nothing to transfer */ I believe '[i]' wants to be moved to 'net' to match the condition below. > + if (optr->net[i].vporttype != VIR_NETDEV_VPORT_PROFILE_NONE) { > + virBufferAsprintf(buf, " <interface index='%d' vporttype='%s'", > + i, virNetDevVPortTypeToString(optr->net[i].vporttype)); > + if (optr->net[i].portdata) { > + virBufferAddLit(buf, ">\n"); > + virBufferEscapeString(buf, " <portdata>%s</portdata>\n", > + optr->net[i].portdata); > + virBufferAddLit(buf, " </interface>\n"); > + } else { > + virBufferAddLit(buf, "/>\n"); > + } > + } > + } > + virBufferAddLit(buf, " </network>\n"); > +} > + > + Maybe if all 'net[i].vporttype' are VIR_NETDEV_VPORT_PROFILE_NONE so there is no <interface/> added, we don't need to add empty <network/> neither. But that just cosmetics and I can live with this version as-is.

> static int > qemuMigrationCookieXMLFormat(struct qemud_driver *driver, > virBufferPtr buf, > @@ -439,6 +577,9 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver, > virBufferAdjustIndent(buf, -2); > } > > + if ((mig->flags & QEMU_MIGRATION_COOKIE_NETWORK) && mig->network) > + qemuMigrationCookieNetworkXMLFormat(buf, mig->network); > + > virBufferAddLit(buf, "</qemu-migration>\n"); > return 0; > } > @@ -516,6 +657,58 @@ error: > } > > > +static qemuMigrationCookieNetworkPtr > +qemuMigrationCookieNetworkXMLParse(xmlXPathContextPtr ctxt) > +{ > + qemuMigrationCookieNetworkPtr optr; > + int i; > + int n; > + xmlNodePtr *interfaces = NULL; > + char *vporttype; > + xmlNodePtr save_ctxt = ctxt->node; > + > + if (VIR_ALLOC(optr) < 0) > + goto no_memory; > + > + if ((n = virXPathNodeSet("./network/interface", ctxt, &interfaces)) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("missing interface information")); > + goto error; > + } > + > + optr->nnets = n; > + if (VIR_ALLOC_N(optr->net, optr->nnets) <0) > + goto no_memory; > + > + for (i = 0; i < n; i++) { > + /* portdata is optional, and may not exist */ > + ctxt->node = interfaces[i]; > + optr->net[i].portdata = virXPathString("string(./portdata[1])", ctxt); > + > + if (!(vporttype = virXMLPropString(interfaces[i], "vporttype"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("missing vporttype attribute in migration data")); > + goto error; > + } > + optr->net[i].vporttype = virNetDevVPortTypeFromString(vporttype); > + } > + > + VIR_FREE(interfaces); > + > +cleanup: > + ctxt->node = save_ctxt; > + return optr; > + > +no_memory: > + virReportOOMError(); > +error: > + VIR_FREE(interfaces); > + qemuMigrationCookieNetworkFree(optr); > + optr = NULL; > + goto cleanup; > +} > + > + > static int > qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, > struct qemud_driver *driver, > @@ -663,6 +856,11 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, > VIR_FREE(nodes); > } > > + if ((flags & QEMU_MIGRATION_COOKIE_NETWORK) && > + virXPathBoolean("count(./network) > 0", ctxt) && > + (!(mig->network = qemuMigrationCookieNetworkXMLParse(ctxt)))) > + goto error; > + > return 0; > > error: > @@ -722,6 +920,10 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig, > qemuMigrationCookieAddPersistent(mig, dom) < 0) > return -1; > > + if (flags & QEMU_MIGRATION_COOKIE_NETWORK && > + qemuMigrationCookieAddNetwork(mig, driver, dom) < 0) > + return -1; > + > if (!(*cookieout = qemuMigrationCookieXMLFormatStr(driver, mig))) > return -1; > > @@ -1084,6 +1286,32 @@ qemuDomainMigrateGraphicsRelocate(struct qemud_driver *driver, > } > > > +static int > +qemuDomainMigrateOPDRelocate(struct qemud_driver *driver ATTRIBUTE_UNUSED, > + virDomainObjPtr vm, > + qemuMigrationCookiePtr cookie) > +{ > + virDomainNetDefPtr netptr ATTRIBUTE_UNUSED; > + int ret = 0; > + int i; > + > + for (i = 0; i < cookie->network->nnets; i++) { > + netptr = vm->def->nets[i]; > + > + switch (cookie->network->net[i].vporttype) { > + case VIR_NETDEV_VPORT_PROFILE_NONE: > + case VIR_NETDEV_VPORT_PROFILE_8021QBG: > + case VIR_NETDEV_VPORT_PROFILE_8021QBH: > + case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: > + default: > + break; > + } > + } > + > + return ret; > +} > + > + > /* This is called for outgoing non-p2p migrations when a connection to the > * client which initiated the migration was closed but we were waiting for it > * to follow up with the next phase, that is, in between > @@ -2029,7 +2257,8 @@ cleanup: > > if (ret == 0 && > qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, > - QEMU_MIGRATION_COOKIE_PERSISTENT ) < 0) > + QEMU_MIGRATION_COOKIE_PERSISTENT | > + QEMU_MIGRATION_COOKIE_NETWORK) < 0) > VIR_WARN("Unable to encode migration cookie"); > > qemuMigrationCookieFree(mig); > @@ -2963,6 +3192,7 @@ qemuMigrationFinish(struct qemud_driver *driver, > > qemuDomainCleanupRemove(vm, qemuMigrationPrepareCleanup); > > + cookie_flags = QEMU_MIGRATION_COOKIE_NETWORK; > if (flags & VIR_MIGRATE_PERSIST_DEST) > cookie_flags |= QEMU_MIGRATION_COOKIE_PERSISTENT; > > @@ -2990,6 +3220,10 @@ qemuMigrationFinish(struct qemud_driver *driver, > goto endjob; > } > > + if (mig->network) > + if (qemuDomainMigrateOPDRelocate(driver, vm, mig) < 0) > + VIR_WARN("unable to provide network data for relocation"); > + > if (flags & VIR_MIGRATE_PERSIST_DEST) { > virDomainDefPtr vmdef; > if (vm->persistent) >