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@xxxxxxxxx>
>> ---
>> 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)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list