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