On Mon, Feb 18, 2013 at 15:38:35 +0100, Michal Privoznik wrote: > This migration cookie is meant for two purposes. The first is to be sent > in begin phase from source to destination to let it know we support new > implementation of VIR_MIGRATE_NON_SHARED_{DISK,INC} so destination can > start NBD server. Then, the second purpose is, destination can let us > know, on which port the NBD server is running. ... > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 36e55d2..82c3f97 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c ... > @@ -121,6 +127,14 @@ struct _qemuMigrationCookieNetwork { > qemuMigrationCookieNetDataPtr net; > }; > > +typedef struct _qemuMigrationCookieNBD qemuMigrationCookieNBD; > +typedef qemuMigrationCookieNBD *qemuMigrationCookieNBDPtr; > +struct _qemuMigrationCookieNBD { > + int port; /* on which port does NBD server listen for incoming data. > + Zero value has special meaning - it is there just to let > + destination know we (the source) do support NBD. */ I think you can drop this note about 0 being special. The support for NBD can detected by an empty <nbd> element in the cookie (i.e., QEMU_MIGRATION_COOKIE_NBD is set in mig->flags and mig->ndb is not NULL). And it seems the parsing code does what I suggest and does not need port='0' attribute in <nbd/> once 9/12 is applied. > +}; > + > typedef struct _qemuMigrationCookie qemuMigrationCookie; > typedef qemuMigrationCookie *qemuMigrationCookiePtr; > struct _qemuMigrationCookie { ... > @@ -837,6 +883,12 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, > goto error; > } > > + /* nbd is optional */ > + if (val == QEMU_MIGRATION_COOKIE_FLAG_NBD) { > + VIR_FREE(str); > + continue; > + } > + This does not make sense at all, just remove this hunk. Features marked as mandatory are always mandatory and NBD is no exception. > if ((flags & (1 << val)) == 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("Unsupported migration cookie feature %s"), > @@ -889,6 +941,27 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, > (!(mig->network = qemuMigrationCookieNetworkXMLParse(ctxt)))) > goto error; > > + if (flags & QEMU_MIGRATION_COOKIE_NBD && > + virXPathBoolean("boolean(./nbd)", ctxt)) { > + char *port; > + > + port = virXPathString("string(./nbd/@port)", ctxt); > + if (port) { > + if (VIR_ALLOC(mig->nbd) < 0) { > + virReportOOMError(); > + goto error; > + } > + if (virStrToLong_i(port, NULL, 10, &mig->nbd->port) < 0) { > + VIR_FREE(port); > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Malformed nbd port '%s'"), > + port); > + goto error; > + } > + VIR_FREE(port); > + } We even have virXPathInt that does the conversion for you, btw. But anyway, sender would send <nbd/> if port == 0 and the receiver would just ignore it since there is no port attribute present. Something's strange here. And you actually fix that in 9/12. > + } > + > virObjectUnref(caps); > return 0; > ... > @@ -1666,7 +1755,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, > origname = NULL; > > if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, > - QEMU_MIGRATION_COOKIE_LOCKSTATE))) > + QEMU_MIGRATION_COOKIE_LOCKSTATE | > + QEMU_MIGRATION_COOKIE_NBD))) > goto cleanup; > > if (qemuMigrationJobStart(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) > @@ -1726,8 +1816,19 @@ done: > else > cookieFlags = QEMU_MIGRATION_COOKIE_GRAPHICS; > > - if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, > - cookieFlags) < 0) { > + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && > + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { I think we should only do this if the source actually asked for NBD, that is mig->nbd is not NULL. > + /* TODO support NBD for TUNNELLED migration */ > + if (flags & VIR_MIGRATE_TUNNELLED) > + VIR_DEBUG("NBD in tunnelled migration is currently not supported"); > + else { > + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; > + priv->nbdPort = 0; > + } > + } > + > + if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, > + cookieoutlen, cookieFlags) < 0) { > /* We could tear down the whole guest here, but > * cookie data is (so far) non-critical, so that > * seems a little harsh. We'll just warn for now. ... > @@ -2241,8 +2353,9 @@ qemuMigrationRun(virQEMUDriverPtr driver, > return -1; > } > > - if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, > - QEMU_MIGRATION_COOKIE_GRAPHICS))) > + mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, > + cookieFlags | QEMU_MIGRATION_COOKIE_GRAPHICS); > + if (!mig) > goto cleanup; > I think we should emit a log message when we support NBD but it was not present in migration cookie. And perhaps clear the flag from cookieFlags. Now I see you did that in 7/12. > if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig) < 0) ... Jirka -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list