On 10/23/2012 08:07 AM, Michal Privoznik wrote:
On 22.10.2012 23:30, Laine Stump wrote:
From: Kyle Mestery <kmestery@cisco.com>
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@cisco.com> --- 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]
Well, what would have saved another level of nesting would be if nnets was part of qemuMigrationCookie instead of qemuMigrationCookieNetwork. I had already removed one extra layer of nesting when I updated Kyle's patches, and thought of making this change as well, but kind of liked the consistency of all the "sub-cookies" being a single pointer that could be checked for NULL: struct _qemuMigrationCookieNetdata { int vporttype; /* enum virNetDevVPortProfile */ char *portdata; }; struct _qemuMigrationCookieNetwork { int nnets; qemuMigrationCookieNetdataPtr net; }; struct _qemuMigrationCookie { ... /* If (flags & QEMU_MIGRATION_COOKIE_GRAPHICS) */ qemuMigrationCookieGraphicsPtr graphics; /* If (flags & QEMU_MIGRATION_COOKIE_PERSISTENT) */ virDomainDefPtr persistent; /* If (flags & QEMU_MIGRATION_COOKIE_NETWORK) */ qemuMigrationCookieNetworkPtr network; }; On the other hand, I noticed at the last minute that there is a LOCKSTATE cookie that *doesn't* follow this pattern: /* If (flags & QEMU_MIGRATION_COOKIE_LOCKSTATE) */ char *lockState; char *lockDriver; so I'd be just as happy with this: struct _qemuMigrationCookie { ... /* If (flags & QEMU_MIGRATION_COOKIE_GRAPHICS) */ qemuMigrationCookieGraphicsPtr graphics; /* If (flags & QEMU_MIGRATION_COOKIE_PERSISTENT) */ virDomainDefPtr persistent; /* If (flags & QEMU_MIGRATION_COOKIE_NETWORK) */ size_t nnets; qemuMigrationCookieNetdataPtr net; }; (i.e. eliminating the qemuMigrationCookieNet object completely)
+ 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.
Not sure how that by itself solves the problem.
+ + 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.
I was a bit bothered by that extra object in there anyway - I'm going to respin to remove it - the array will be directly off the main cookie object.
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)