On 09/21/2012 05:16 PM, Kyle Mestery wrote:

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.

Functionally this all looks good (and sounds like it lives up to that in practice :-). There are some code style issues, though...

Signed-off-by: Kyle Mestery <kmestery@cisco.com> --- src/qemu/qemu_migration.c | 278 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 276 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8e85875..06981db 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -70,6 +70,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 +78,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 +97,35 @@ struct _qemuMigrationCookieGraphics { char *tlsSubject; };

+VIR_ENUM_DECL(virInterface); +VIR_ENUM_IMPL(virInterface, VIR_NETDEV_VPORT_PROFILE_LAST, + "none", + "802.1QBg", + "802.1QBh", + "openvswitch");

(I'd never before considered using VIR_ENUM_*() and giving an enum type identifier that didn't match the enum being translated....) Since you're using the enum virNetDevVPortProfile for these values anyway, you should just use the already-existing functions: virNetDevVPortTypeFromString virNetDevVPortTypeToString (these are declared/defined in virnetdevvportprofile.h, and included in libvirt_private.syms).

+ +typedef struct _qemuMigrationCookieNetdata qemuMigrationCookieNetdata; +typedef qemuMigrationCookieNetdata *qemuMigrationCookieNetdataPtr; +struct _qemuMigrationCookieNetdata { + /* What type is each interace ? virInterface above ... */ + char *interfacetype;

1) Since the xml attribute is called "vporttype", you should give the variable in the struct the same name, to avoid confusion. 2) Internally, we usually store the enum value rather than the string, and translate to/from string form during XML parse/format. (For some reason, normal usage is to specify it in the struct as an int, with a comment giving the actual type: int vporttype; /* enum virNetDevVPortProfile */ I'm not sure why that is, to be truthful - Eric or Dan?)

+ + /* + * 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 +151,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 +166,28 @@ static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap) }

+static void qemuMigrationCookieNetworkFree(qemuMigrationCookieNetworkPtr + network) +{ + int i; + + if (!network) + return; + + for (i = 0; i < network->nnets; i++) { + if (network->net[i]) { + if (network->net[i]->interfacetype) + VIR_FREE(network->net[i]->interfacetype);

VIR_FREE() automatically checks for NULL, and is a NOP if the pointer is NULL (it also sets the pointer to NULL when it's finished, so calling it twice doesn't create a problem. "make syntax-check" will actually find such redundant null-check-before-free and complain about it. So, you should remove all checks for NULL before calling VIR_FREE() (of course you need the outside "if (network->net[i])" to remain, though :-)

+ if (network->net[i]->portdata) + VIR_FREE(network->net[i]->portdata); + VIR_FREE(network->net[i]); + } + } + VIR_FREE(network->net); + VIR_FREE(network); +} + + static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig) { if (!mig) @@ -140,6 +196,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); + } + VIR_FREE(mig->localHostname); VIR_FREE(mig->remoteHostname); VIR_FREE(mig->name); @@ -256,6 +316,65 @@ error: }

+static qemuMigrationCookieNetworkPtr +qemuMigrationCookieNetworkAlloc(struct qemud_driver *driver ATTRIBUTE_UNUSED, + virDomainDefPtr def) +{ + qemuMigrationCookieNetworkPtr mig; + int i; + virDomainNetDefPtr netptr ATTRIBUTE_UNUSED; + const char *interfacetype; + + if (VIR_ALLOC(mig) < 0) + goto no_memory; + + mig->nnets = def->nnets; + + if (VIR_ALLOC_N(mig->net, def->nnets) <0) + goto no_memory; + + for (i = 0; i < def->nnets; i++) { + virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(def->nets[i]); + netptr = def->nets[i]; + + if (vport) { + if (VIR_ALLOC(mig->net[i]) < 0) + goto no_memory; + if (VIR_ALLOC(mig->net[i]->interfacetype) < 0) + goto no_memory; + + interfacetype = virInterfaceTypeToString(vport->virtPortType); + strncpy(mig->net[i]->interfacetype, interfacetype, strlen(interfacetype));

Right here is where you should just do "mig->net[i]->vporttype = vport->virtPortType" instead of translating it into a string.

+ + switch (vport->virtPortType) { + case VIR_NETDEV_VPORT_PROFILE_NONE: + mig->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_8021QBG: + mig->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_8021QBH: + mig->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: + mig->net[i]->portdata = NULL; + break; + default: + mig->net[i]->portdata = NULL; + break;

Applicable to all of the above: VIR_ALLOC guarantees that all allocated memory is initialized to 0's, so individual initialization of properties to 0/NULL isn't required (and having it there mistakenly implies to new coders that it *is* necessary). So you should remove this initialization.

+ } + } + } + + return mig; + +no_memory: + virReportOOMError(); + qemuMigrationCookieNetworkFree(mig); + return NULL; +} + + static qemuMigrationCookiePtr qemuMigrationCookieNew(virDomainObjPtr dom) { @@ -370,6 +489,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 >= 1) {

(I would have just said " > 0", but I suppose there's no practical difference :-)

+ if (!(mig->network = qemuMigrationCookieNetworkAlloc( + driver, dom->def))) + return -1; + mig->flags |= QEMU_MIGRATION_COOKIE_NETWORK; + } + + return 0; +} +

static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf, qemuMigrationCookieGraphicsPtr grap) @@ -389,6 +529,28 @@ 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->interfacetype[i] is not set, there is nothing to transfer */ + if (optr->net[i] && optr->net[i]->interfacetype) { + if (!optr->net[i]->portdata) + virBufferAsprintf(buf, " <interface index='%d' vporttype='%s'/>\n", + i, optr->net[i]->interfacetype);

... and here is where you would use "virNetDevVPortTypeToString(optr->net[i]->vporttype)" (to account for storing the enum directly in the object rather than a string)

+ else + virBufferAsprintf(buf, " <interface index='%d' vporttype='%s' portdata='%s'/>\n", + i, optr->net[i]->interfacetype, optr->net[i]->portdata);

Actually, is it necessary to add the <interface> element for an interface that has nothing other than the vporttype? Maybe you only need the 2nd virBufferAsprintf(), with a preceding "if (optr->net[i]->portdata)". Even if you decide that you should always have the <interface> element regardless of presence of portdata, you could eliminate the duplicated string formatting by doing it like this: virBufferAsprintf(buf, " <interface index='%d' vporttype='%s'", i, optr->net[i]->interfacetype); if (optr->net[i]->portdata) virBufferAsprintf(buf, " portdata='%s'", optr->net[i]->portdata); virBufferAddLit(buf, "/>\n"); This would also allow for easier expansion if/when more option attributes are added to <interface> in the future.

+ } + } + + virBufferAddLit(buf, " </network>\n"); +} + + static int qemuMigrationCookieXMLFormat(struct qemud_driver *driver, virBufferPtr buf, @@ -439,6 +601,10 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver, virBufferAdjustIndent(buf, -2); }

+ if ((mig->flags & QEMU_MIGRATION_COOKIE_NETWORK) && + mig->network)

I know it's serious nit-picking, but since the above two lines add up to < 80 columns, I'd rather have them on the same line (and if you do have an if () expression that takes up > 1 line, I like to put { } around the following statement even if it's not compound, just to avoid confusion).

+ qemuMigrationCookieNetworkXMLFormat(buf, mig->network); + virBufferAddLit(buf, "</qemu-migration>\n"); return 0; } @@ -516,6 +682,60 @@ error: }

+static qemuMigrationCookieNetworkPtr +qemuMigrationCookieNetworkXMLParse(xmlXPathContextPtr ctxt) +{ + qemuMigrationCookieNetworkPtr optr; + int i; + int n; + xmlNodePtr *interfaces = NULL; + + if (VIR_ALLOC(optr) < 0) + goto no_memory; + + if ((n = virXPathNodeSet("./network/interface", ctxt, &interfaces)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing interace information")); + goto error; + } + + optr->nnets = n; + if (VIR_ALLOC_N(optr->net, optr->nnets) <0) + goto no_memory; + + for (i = 0; i < n; i++) { + if (VIR_ALLOC(optr->net[i]) < 0) + goto no_memory; + if (VIR_ALLOC(optr->net[i]->portdata) < 0) + goto no_memory; + + if (VIR_ALLOC(optr->net[i]->interfacetype) < 0) + goto no_memory; + + /* portdata is optional, and may not exist */ + optr->net[i]->portdata = virXMLPropString(interfaces[i], "portdata"); + + if (!(optr->net[i]->interfacetype = virXMLPropString(interfaces[i], "vporttype"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing vporttype attribute in migration data")); + goto error; + } + } + + VIR_FREE(interfaces); + + return optr; + +no_memory: + virReportOOMError(); +error: + if (interfaces)

Another redundant NULL check - just unconditionally call VIR_FREE ("make syntax-check" would have caught this)

+ VIR_FREE(interfaces); + qemuMigrationCookieNetworkFree(optr); + return NULL; +} + + static int qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, struct qemud_driver *driver, @@ -662,6 +882,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: @@ -721,6 +946,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;

@@ -1050,6 +1279,45 @@ 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; + int interfacetype; + + for (i = 0; i < cookie->network->nnets; i++) { + netptr = vm->def->nets[i]; + + if (cookie->network->net[i]->interfacetype) { + interfacetype = virInterfaceTypeFromString(cookie->network->net[i]->interfacetype); + switch (interfacetype) { + case VIR_NETDEV_VPORT_PROFILE_NONE: + cookie->network->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_8021QBG: + cookie->network->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_8021QBH: + cookie->network->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: + cookie->network->net[i]->portdata = NULL; + break; + default: + cookie->network->net[i]->portdata = NULL; + break;

Another case of unnecessary NULL initialization - either portdata has never been used and is guaranteed to be NULL already, or if it's not NULL, then it's pointing to something that needs to be freed (I haven't checked the use-case for this function to see which is the case).

+ } + } + } + + 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 @@ -1994,7 +2262,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); @@ -2929,6 +3198,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;

@@ -2956,6 +3226,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)

On Sep 28, 2012, at 11:14 AM, Laine Stump wrote:

On 09/21/2012 05:16 PM, Kyle Mestery wrote:

...

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.

Functionally this all looks good (and sounds like it lives up to that in practice :-). There are some code style issues, though…

Thanks for all the comments Laine. I'll address them all and send out new versions of the patches ASAP. Kyle

...

Signed-off-by: Kyle Mestery <kmestery@cisco.com> --- src/qemu/qemu_migration.c | 278 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 276 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8e85875..06981db 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -70,6 +70,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 +78,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 +97,35 @@ struct _qemuMigrationCookieGraphics { char *tlsSubject; };

+VIR_ENUM_DECL(virInterface); +VIR_ENUM_IMPL(virInterface, VIR_NETDEV_VPORT_PROFILE_LAST, + "none", + "802.1QBg", + "802.1QBh", + "openvswitch");

(I'd never before considered using VIR_ENUM_*() and giving an enum type identifier that didn't match the enum being translated....)

Since you're using the enum virNetDevVPortProfile for these values anyway, you should just use the already-existing functions:

virNetDevVPortTypeFromString virNetDevVPortTypeToString

(these are declared/defined in virnetdevvportprofile.h, and included in libvirt_private.syms).

...

+ +typedef struct _qemuMigrationCookieNetdata qemuMigrationCookieNetdata; +typedef qemuMigrationCookieNetdata *qemuMigrationCookieNetdataPtr; +struct _qemuMigrationCookieNetdata { + /* What type is each interace ? virInterface above ... */ + char *interfacetype;

1) Since the xml attribute is called "vporttype", you should give the variable in the struct the same name, to avoid confusion.

2) Internally, we usually store the enum value rather than the string, and translate to/from string form during XML parse/format. (For some reason, normal usage is to specify it in the struct as an int, with a comment giving the actual type:

int vporttype; /* enum virNetDevVPortProfile */

I'm not sure why that is, to be truthful - Eric or Dan?)

...

+ + /* + * 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 +151,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 +166,28 @@ static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap) }

+static void qemuMigrationCookieNetworkFree(qemuMigrationCookieNetworkPtr + network) +{ + int i; + + if (!network) + return; + + for (i = 0; i < network->nnets; i++) { + if (network->net[i]) { + if (network->net[i]->interfacetype) + VIR_FREE(network->net[i]->interfacetype);

VIR_FREE() automatically checks for NULL, and is a NOP if the pointer is NULL (it also sets the pointer to NULL when it's finished, so calling it twice doesn't create a problem. "make syntax-check" will actually find such redundant null-check-before-free and complain about it. So, you should remove all checks for NULL before calling VIR_FREE()

(of course you need the outside "if (network->net[i])" to remain, though :-)

...

+ if (network->net[i]->portdata) + VIR_FREE(network->net[i]->portdata); + VIR_FREE(network->net[i]); + } + } + VIR_FREE(network->net); + VIR_FREE(network); +} + + static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig) { if (!mig) @@ -140,6 +196,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); + } + VIR_FREE(mig->localHostname); VIR_FREE(mig->remoteHostname); VIR_FREE(mig->name); @@ -256,6 +316,65 @@ error: }

+static qemuMigrationCookieNetworkPtr +qemuMigrationCookieNetworkAlloc(struct qemud_driver *driver ATTRIBUTE_UNUSED, + virDomainDefPtr def) +{ + qemuMigrationCookieNetworkPtr mig; + int i; + virDomainNetDefPtr netptr ATTRIBUTE_UNUSED; + const char *interfacetype; + + if (VIR_ALLOC(mig) < 0) + goto no_memory; + + mig->nnets = def->nnets; + + if (VIR_ALLOC_N(mig->net, def->nnets) <0) + goto no_memory; + + for (i = 0; i < def->nnets; i++) { + virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(def->nets[i]); + netptr = def->nets[i]; + + if (vport) { + if (VIR_ALLOC(mig->net[i]) < 0) + goto no_memory; + if (VIR_ALLOC(mig->net[i]->interfacetype) < 0) + goto no_memory; + + interfacetype = virInterfaceTypeToString(vport->virtPortType); + strncpy(mig->net[i]->interfacetype, interfacetype, strlen(interfacetype));

Right here is where you should just do "mig->net[i]->vporttype = vport->virtPortType" instead of translating it into a string.

...

+ + switch (vport->virtPortType) { + case VIR_NETDEV_VPORT_PROFILE_NONE: + mig->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_8021QBG: + mig->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_8021QBH: + mig->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: + mig->net[i]->portdata = NULL; + break; + default: + mig->net[i]->portdata = NULL; + break;

Applicable to all of the above: VIR_ALLOC guarantees that all allocated memory is initialized to 0's, so individual initialization of properties to 0/NULL isn't required (and having it there mistakenly implies to new coders that it *is* necessary). So you should remove this initialization.

...

+ } + } + } + + return mig; + +no_memory: + virReportOOMError(); + qemuMigrationCookieNetworkFree(mig); + return NULL; +} + + static qemuMigrationCookiePtr qemuMigrationCookieNew(virDomainObjPtr dom) { @@ -370,6 +489,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 >= 1) {

(I would have just said " > 0", but I suppose there's no practical difference :-)

...

+ if (!(mig->network = qemuMigrationCookieNetworkAlloc( + driver, dom->def))) + return -1; + mig->flags |= QEMU_MIGRATION_COOKIE_NETWORK; + } + + return 0; +} +

static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf, qemuMigrationCookieGraphicsPtr grap) @@ -389,6 +529,28 @@ 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->interfacetype[i] is not set, there is nothing to transfer */ + if (optr->net[i] && optr->net[i]->interfacetype) { + if (!optr->net[i]->portdata) + virBufferAsprintf(buf, " <interface index='%d' vporttype='%s'/>\n", + i, optr->net[i]->interfacetype);

... and here is where you would use "virNetDevVPortTypeToString(optr->net[i]->vporttype)" (to account for storing the enum directly in the object rather than a string)

...

+ else + virBufferAsprintf(buf, " <interface index='%d' vporttype='%s' portdata='%s'/>\n", + i, optr->net[i]->interfacetype, optr->net[i]->portdata);

Actually, is it necessary to add the <interface> element for an interface that has nothing other than the vporttype? Maybe you only need the 2nd virBufferAsprintf(), with a preceding "if (optr->net[i]->portdata)". Even if you decide that you should always have the <interface> element regardless of presence of portdata, you could eliminate the duplicated string formatting by doing it like this:

virBufferAsprintf(buf, " <interface index='%d' vporttype='%s'", i, optr->net[i]->interfacetype); if (optr->net[i]->portdata) virBufferAsprintf(buf, " portdata='%s'", optr->net[i]->portdata); virBufferAddLit(buf, "/>\n");

This would also allow for easier expansion if/when more option attributes are added to <interface> in the future.

...

+ } + } + + virBufferAddLit(buf, " </network>\n"); +} + + static int qemuMigrationCookieXMLFormat(struct qemud_driver *driver, virBufferPtr buf, @@ -439,6 +601,10 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver, virBufferAdjustIndent(buf, -2); }

+ if ((mig->flags & QEMU_MIGRATION_COOKIE_NETWORK) && + mig->network)

I know it's serious nit-picking, but since the above two lines add up to < 80 columns, I'd rather have them on the same line (and if you do have an if () expression that takes up > 1 line, I like to put { } around the following statement even if it's not compound, just to avoid confusion).

...

+ qemuMigrationCookieNetworkXMLFormat(buf, mig->network); + virBufferAddLit(buf, "</qemu-migration>\n"); return 0; } @@ -516,6 +682,60 @@ error: }

+static qemuMigrationCookieNetworkPtr +qemuMigrationCookieNetworkXMLParse(xmlXPathContextPtr ctxt) +{ + qemuMigrationCookieNetworkPtr optr; + int i; + int n; + xmlNodePtr *interfaces = NULL; + + if (VIR_ALLOC(optr) < 0) + goto no_memory; + + if ((n = virXPathNodeSet("./network/interface", ctxt, &interfaces)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing interace information")); + goto error; + } + + optr->nnets = n; + if (VIR_ALLOC_N(optr->net, optr->nnets) <0) + goto no_memory; + + for (i = 0; i < n; i++) { + if (VIR_ALLOC(optr->net[i]) < 0) + goto no_memory; + if (VIR_ALLOC(optr->net[i]->portdata) < 0) + goto no_memory; + + if (VIR_ALLOC(optr->net[i]->interfacetype) < 0) + goto no_memory; + + /* portdata is optional, and may not exist */ + optr->net[i]->portdata = virXMLPropString(interfaces[i], "portdata"); + + if (!(optr->net[i]->interfacetype = virXMLPropString(interfaces[i], "vporttype"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing vporttype attribute in migration data")); + goto error; + } + } + + VIR_FREE(interfaces); + + return optr; + +no_memory: + virReportOOMError(); +error: + if (interfaces)

Another redundant NULL check - just unconditionally call VIR_FREE ("make syntax-check" would have caught this)

...

+ VIR_FREE(interfaces); + qemuMigrationCookieNetworkFree(optr); + return NULL; +} + + static int qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, struct qemud_driver *driver, @@ -662,6 +882,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: @@ -721,6 +946,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;

@@ -1050,6 +1279,45 @@ 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; + int interfacetype; + + for (i = 0; i < cookie->network->nnets; i++) { + netptr = vm->def->nets[i]; + + if (cookie->network->net[i]->interfacetype) { + interfacetype = virInterfaceTypeFromString(cookie->network->net[i]->interfacetype); + switch (interfacetype) { + case VIR_NETDEV_VPORT_PROFILE_NONE: + cookie->network->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_8021QBG: + cookie->network->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_8021QBH: + cookie->network->net[i]->portdata = NULL; + break; + case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: + cookie->network->net[i]->portdata = NULL; + break; + default: + cookie->network->net[i]->portdata = NULL; + break;

Another case of unnecessary NULL initialization - either portdata has never been used and is guaranteed to be NULL already, or if it's not NULL, then it's pointing to something that needs to be freed (I haven't checked the use-case for this function to see which is the case).

...

+ } + } + } + + 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 @@ -1994,7 +2262,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); @@ -2929,6 +3198,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;

@@ -2956,6 +3226,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)

On 09/21/2012 05:16 PM, Kyle Mestery wrote:

Add utility functions for Open vSwitch to both save per-port data before a live migration, and restore the per-port data after a live migration.

Signed-off-by: Kyle Mestery <kmestery@cisco.com> --- src/libvirt_private.syms | 2 ++ src/util/virnetdevopenvswitch.c | 70 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 6 ++++ 3 files changed, 78 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0b6068d..54c591b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1479,7 +1479,9 @@ virNetDevMacVLanVPortProfileRegisterCallback;

# virnetdevopenvswitch.h virNetDevOpenvswitchAddPort; +virNetDevOpenvswitchGetMigrateData; virNetDevOpenvswitchRemovePort; +virNetDevOpenvswitchSetMigrateData;

# virnetdevtap.h diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index a6993b6..c30cbaa 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -179,3 +179,73 @@ int virNetDevOpenvswitchRemovePort(const char *brname ATTRIBUTE_UNUSED, const ch virCommandFree(cmd); return ret; } + +/** + * virNetDevOpenvswitchGetMigrateData: + * @migrate: a pointer to store the data into, allocated by this function + * @ifname: name of the interface for which data is being migrated + * + * Allocates data to be migrated specific to Open vSwitch + * + * Returns 0 in case of success or -1 in case of failure + */ +int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname) +{ + virCommandPtr cmd = NULL; + int ret = 0; + + cmd = virCommandNewArgList(OVSVSCTL, "get", "Interface", + ifname, "external_ids:PortData", NULL); + + virCommandSetOutputBuffer(cmd, migrate); + + /* Run the command */ + if (virCommandRun(cmd, NULL) < 0) { + virReportSystemError(VIR_ERR_INTERNAL_ERROR, + _("Unable to run command to get OVS port data for " + "interface %s"), ifname); + ret = -1; + goto error; + } + + /* Wipeout the newline */ + (*migrate)[strlen(*migrate) - 1] = '\0'; + +error: + return ret; +} + +/** + * virNetDevOpenvswitchSetMigrateData: + * @migrate: the data which was transferred during migration + * @ifname: the name of the interface the data is associated with + * + * Repopulates OVS per-port data on destination host + * + * Returns 0 in case of success or -1 in case of failure + */ +int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname) +{ + virCommandPtr cmd = NULL; + int ret = 0; + virBufferPtr buf; + + if (VIR_ALLOC(buf) < 0) { + ret = -1; + goto error; + } + + virBufferAsprintf(buf, "external_ids:PortData=%s", migrate); + + cmd = virCommandNewArgList(OVSVSCTL, "set", "Interface", ifname, + virBufferCurrentContent(buf), NULL);

You need to add the "--timeout=5" option as I did for add-port and del-port, to avoid an infinite wait when ovs-vswitchd isn't running. ACK with that change.

+ /* Run the command */ + if ((ret = virCommandRun(cmd, NULL)) < 0) { + virReportSystemError(VIR_ERR_INTERNAL_ERROR, + _("Unable to run command to set OVS port data for " + "interface %s"), ifname); + } + +error: + return ret; +} diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 7e5b618..147cd6f 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -42,4 +42,10 @@ int virNetDevOpenvswitchAddPort(const char *brname, int virNetDevOpenvswitchRemovePort(const char *brname, const char *ifname) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;

+int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + +int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_NETDEV_OPENVSWITCH_H__ */

On 09/21/2012 05:16 PM, Kyle Mestery wrote:

Transport Open vSwitch per-port data during live migration by using the utility functions virNetDevOpenvswitchGetMigrateData() and virNetDevOpenvswitchSetMigrateData().

I like how the first part is all re-usable infrastructure, and the final patch that actually makes things work is so small! That implies that it will be easy to add other new functionality in the future...

Signed-off-by: Kyle Mestery <kmestery@cisco.com> --- src/qemu/qemu_migration.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 06981db..33cdb20 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -322,7 +322,7 @@ qemuMigrationCookieNetworkAlloc(struct qemud_driver *driver ATTRIBUTE_UNUSED, { qemuMigrationCookieNetworkPtr mig; int i; - virDomainNetDefPtr netptr ATTRIBUTE_UNUSED; + virDomainNetDefPtr netptr; const char *interfacetype;

if (VIR_ALLOC(mig) < 0) @@ -357,7 +357,13 @@ qemuMigrationCookieNetworkAlloc(struct qemud_driver *driver ATTRIBUTE_UNUSED, mig->net[i]->portdata = NULL; break; case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: - mig->net[i]->portdata = NULL; + if (virNetDevOpenvswitchGetMigrateData(&mig->net[i]->portdata, + netptr->ifname) != 0) { + virReportSystemError(VIR_ERR_INTERNAL_ERROR, + _("Unable to run command to get OVS port data for " + "interface %s"), netptr->ifname); + goto error; + } break; default: mig->net[i]->portdata = NULL; @@ -366,6 +372,7 @@ qemuMigrationCookieNetworkAlloc(struct qemud_driver *driver ATTRIBUTE_UNUSED, } }

+error: return mig;

no_memory: @@ -1284,7 +1291,7 @@ qemuDomainMigrateOPDRelocate(struct qemud_driver *driver ATTRIBUTE_UNUSED, virDomainObjPtr vm, qemuMigrationCookiePtr cookie) { - virDomainNetDefPtr netptr ATTRIBUTE_UNUSED; + virDomainNetDefPtr netptr; int ret = 0;

These days we tend to 1) initialize "ret = -1" (assume failure), call the early exit label "cleanup" rather than "error" (if a successful exit will run through that label - it avoids confusion when someone reads the code later), and put a "ret = 0;" on the line just before "cleanup:". That way, you don't need to put in all the "ret = -1;" lines whenever there is a failure. ACK with that change (and after re-basing to account for the changes requested in 1/3).

int i; int interfacetype; @@ -1305,7 +1312,14 @@ qemuDomainMigrateOPDRelocate(struct qemud_driver *driver ATTRIBUTE_UNUSED, cookie->network->net[i]->portdata = NULL; break; case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: - cookie->network->net[i]->portdata = NULL; + if (virNetDevOpenvswitchSetMigrateData(cookie->network->net[i]->portdata, + netptr->ifname) != 0) { + virReportSystemError(VIR_ERR_INTERNAL_ERROR, + _("Unable to run command to set OVS port data for " + "interface %s"), netptr->ifname); + ret = -1; + goto error; + } break; default: cookie->network->net[i]->portdata = NULL; @@ -1314,6 +1328,7 @@ qemuDomainMigrateOPDRelocate(struct qemud_driver *driver ATTRIBUTE_UNUSED, } }

+error: return ret; }