[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 data during migration



On Oct 22, 2012, at 4:21 PM, Laine Stump <laine laine org> wrote:
> On 10/01/2012 11:18 AM, 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.
> 
> I changed this patch to 1) put the portdata in a separate element rather
> than having an extremely long attribute, 2) escape the portdata to avoid
> "surprises" if the data happened to have a character that has a
> syntactical meaning for xml, and 3) clean up miscellaneous other
> problems I've noted below.
> 
> Because I'm unable to test these changes myself (and they are more than
> trivial), I'm reposting the series. Please test it and report back
> whether or not it works (ie ACK or NACK). If ACK, then I'll push the
> updated patches.
> 
This is great, thanks Laine! I just now see your updated patches, I will pull those down,
apply, rebuild and let you know how my testing goes with a ACK/NACK reply.

Thanks!
Kyle

>> 
>> Signed-off-by: Kyle Mestery <kmestery cisco com>
>> ---
>> src/qemu/qemu_migration.c | 246 +++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 244 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index db69a0a..a17ccbd 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,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;
> 
> I changed this to "qemuMigratiponCookieNetdataPtr net;" to reduce the
> levels of indirection. The effect is that this is now points to an array
> of qemuMigrationCookieNetdata, rather than pointing to an array of
> pointers to qemu.....Netdata. All the rest of the code (in this patch
> and in 3/3) was adjusted accordingly.
> 
>> +};
>> +
>> typedef struct _qemuMigrationCookie qemuMigrationCookie;
>> typedef qemuMigrationCookie *qemuMigrationCookiePtr;
>> struct _qemuMigrationCookie {
>> @@ -120,6 +143,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 +158,25 @@ static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap)
>> }
>> 
>> 
>> +static void qemuMigrationCookieNetworkFree(qemuMigrationCookieNetworkPtr
>> +                                               network)
> 
> Here and in a few other places, I changed the formatting to this:
> 
> static void
> functionName(xxxx xxx,
>             xxxx xxx)
> 
> (note the return type and function name on separate lines, and the first
> characters of the arg types lining up rather than being offset).
> 
>> +{
>> +    int i;
>> +
>> +    if (!network)
>> +        return;
>> +
>> +    for (i = 0; i < network->nnets; i++) {
>> +        if (network->net[i]) {
>> +            VIR_FREE(network->net[i]->portdata);
>> +            VIR_FREE(network->net[i]);
>> +        }
>> +    }
> 
> This was adjusted for the one layer less of indirection.
> 
>> +    VIR_FREE(network->net);
>> +    VIR_FREE(network);
>> +}
>> +
>> +
>> static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig)
>> {
>>     if (!mig)
>> @@ -140,6 +185,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 +305,57 @@ error:
>> }
>> 
>> 
>> +static qemuMigrationCookieNetworkPtr
>> +qemuMigrationCookieNetworkAlloc(struct qemud_driver *driver ATTRIBUTE_UNUSED,
>> +                                    virDomainDefPtr def)
>> +{
>> +    qemuMigrationCookieNetworkPtr mig;
>> +    int i;
>> +    virDomainNetDefPtr netptr ATTRIBUTE_UNUSED;
> 
> I moved this into the for loop and eliminated the ATTRIBUTE_UNUSED (and
> actually *do* use it, as an arg to virDomainNetGetActualVirtPortProfile)
> 
>> +
>> +    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;
>> +
>> +            mig->net[i]->vporttype = vport->virtPortType;
>> +
>> +            switch (vport->virtPortType) {
>> +            case VIR_NETDEV_VPORT_PROFILE_NONE:
>> +                break;
>> +            case VIR_NETDEV_VPORT_PROFILE_8021QBG:
>> +                break;
>> +            case VIR_NETDEV_VPORT_PROFILE_8021QBH:
>> +                break;
>> +            case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
>> +                break;
> 
> All those breaks were combined.
> 
>> +            default:
>> +                mig->net[i]->portdata = NULL;
> 
> This is unnecessary, as all newly allocated memory is initialized to 0.
> 
>> +                break;
>> +            }
>> +        }
>> +    }
>> +
>> +    return mig;
>> +
>> +no_memory:
>> +    virReportOOMError();
>> +    qemuMigrationCookieNetworkFree(mig);
>> +    return NULL;
>> +}
>> +
>> +
>> static qemuMigrationCookiePtr
>> qemuMigrationCookieNew(virDomainObjPtr dom)
>> {
>> @@ -370,6 +470,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) {
>> +        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 +510,27 @@ 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 */
>> +        if (optr->net[i] && 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)
>> +                virBufferAsprintf(buf, " portdata='%s'", optr->net[i]->portdata);
> 
> Changed this to fix the indent of the <interface> line (should be 4
> spaces) and put portdata into its own subelement, as well as escaping
> the portdata as its formatted into the buffer.
> 
>> +            virBufferAddLit(buf, "/>\n");
>> +        }
>> +    }
>> +
>> +    virBufferAddLit(buf, "  </network>\n");
>> +}
>> +
>> +
>> static int
>> qemuMigrationCookieXMLFormat(struct qemud_driver *driver,
>>                              virBufferPtr buf,
>> @@ -439,6 +581,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 +661,58 @@ error:
>> }
>> 
>> 
>> +static qemuMigrationCookieNetworkPtr
>> +qemuMigrationCookieNetworkXMLParse(xmlXPathContextPtr ctxt)
>> +{
>> +    qemuMigrationCookieNetworkPtr optr;
>> +    int i;
>> +    int n;
>> +    xmlNodePtr *interfaces = NULL;
>> +    char *vporttype;
>> +
>> +    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"));
> 
> s/interrace/interface/ :-)
> 
> 
>> +        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;
> 
> This alloc is no longer necessary due to changing the data structure.
> 
>> +        if (VIR_ALLOC(optr->net[i]->portdata) < 0)
>> +            goto no_memory;
> 
> This alloc never was proper - it's just allocating a single character
> that is later leaked.
> 
> 
>> +
>> +        /* portdata is optional, and may not exist */
>> +        optr->net[i]->portdata = virXMLPropString(interfaces[i], "portdata");
> 
> Get this with virXPathString("string(./portdata[1])", ctxt) instead
> (after setting ctxt->node = interfaces[i])
> 
>> +
>> +        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);
>> +
>> +    return optr;
>> +
>> +no_memory:
>> +    virReportOOMError();
>> +error:
>> +    VIR_FREE(interfaces);
>> +    qemuMigrationCookieNetworkFree(optr);
>> +    return NULL;
>> +}
>> +
>> +
>> static int
>> qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig,
>>                             struct qemud_driver *driver,
>> @@ -662,6 +859,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 +923,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;
>> 
>> @@ -1067,6 +1273,36 @@ 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:
>> +            break;
>> +        case VIR_NETDEV_VPORT_PROFILE_8021QBG:
>> +            break;
>> +        case VIR_NETDEV_VPORT_PROFILE_8021QBH:
>> +            break;
>> +        case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
>> +            break;
>> +        default:
>> +            break;
> 
> I combined all the breaks.
> 
>> +        }
>> +    }
>> +
>> +    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
>> @@ -2011,7 +2247,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);
>> @@ -2946,6 +3183,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;
>> 
>> @@ -2973,6 +3211,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)
> 
> Otherwise good. counter-patch coming up!
> 




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