[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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)





[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]