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

On 10/23/2012 08:07 AM, Michal Privoznik wrote: > On 22.10.2012 23:30, Laine Stump wrote: >> From: Kyle Mestery <kmestery(a)cisco.com&gt; >> >> 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(a)cisco.com&gt; >> --- >> 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)