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