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

[Laine Stump]

On 10/23/2012 12:11 PM, Laine Stump wrote:
> On 10/23/2012 08:07 AM, Michal Privoznik wrote:
>> On 22.10.2012 23:30, Laine Stump wrote:
>>> From: Kyle Mestery <kmestery at 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 at 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)

Actually, I've changed my mind, and want to leave it as is - modifying
it as above would make it just slightly more efficient, but would
actually create some ugly/extra code in a few places that make it (to
me) not worthwhile. (I know, I could do the trick of having
qemuMigrationCookieNetwork be "... size_t nnets;
qemuMigrationCookieNetdataPtr net[0]; then allocate an odd sized block
of data, but that is even uglier).

I'm making the other changes you suggested though.