Since as the code indicates it doesn't work yet, so let's be explicit about it. --- src/qemu/qemu_migration.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9ac9be4..ffc86a4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1900,12 +1900,13 @@ char *qemuMigrationBegin(virQEMUDriverPtr driver, if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { /* 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 (flags & VIR_MIGRATE_TUNNELLED) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("NBD in tunnelled migration is currently not supported")); + goto cleanup; } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + priv->nbdPort = 0; } if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) @@ -2200,16 +2201,11 @@ done: if (mig->nbd && flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { - /* TODO support NBD for TUNNELLED migration */ - if (flags & VIR_MIGRATE_TUNNELLED) - VIR_DEBUG("NBD in tunnelled migration is currently not supported"); - else { - if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { - /* error already reported */ - goto endjob; - } - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { + /* error already reported */ + goto endjob; } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; } if (qemuMigrationBakeCookie(mig, driver, vm, cookieout,

On 05/29/2013 03:16 PM, Michal Privoznik wrote: > On 28.05.2013 22:44, Cole Robinson wrote: >> Since as the code indicates it doesn't work yet, so let's be >> explicit about it. >> --- >> src/qemu/qemu_migration.c | 24 ++++++++++-------------- >> 1 file changed, 10 insertions(+), 14 deletions(-) >> >> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c >> index 9ac9be4..ffc86a4 100644 >> --- a/src/qemu/qemu_migration.c >> +++ b/src/qemu/qemu_migration.c >> @@ -1900,12 +1900,13 @@ char *qemuMigrationBegin(virQEMUDriverPtr driver, >> if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && >> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { >> /* 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 (flags & VIR_MIGRATE_TUNNELLED) { >> + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", >> + _("NBD in tunnelled migration is currently not supported")); >> + goto cleanup; >> } >> + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; >> + priv->nbdPort = 0; >> } >> >> if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) >> @@ -2200,16 +2201,11 @@ done: >> if (mig->nbd && >> flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && >> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { >> - /* TODO support NBD for TUNNELLED migration */ >> - if (flags & VIR_MIGRATE_TUNNELLED) >> - VIR_DEBUG("NBD in tunnelled migration is currently not supported"); >> - else { >> - if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { >> - /* error already reported */ >> - goto endjob; >> - } >> - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; >> + if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { >> + /* error already reported */ >> + goto endjob; >> } >> + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; >> } >> >> if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, >> > > I know you've already pushed this but I don't think this is desired. > There are currently two ways to copy the storage during migration: > > 1) new one using nbd server > 2) old one, using an 'copy_storage' attribute to 'migrate' command > > The first one is preferred and turned on whenever libvirt detects both > sides support it. There's no way for a user to enforce one or another > way of copying the storage. So if the old way works even for tunnelled > migration, we should fall back to using the old way rather then erroring > out. The debug message was really just a debug message :) > Indeed I didn't consider that, sorry. However given that the old storage migration support is 1) considered not very useful and 2) more or less deprecated in upstream qemu, I still think my patch is an improvement. The fact that a common combination of cli options falls back to a kinda-working-but-not-really-supported version with virtually no indication to the user is pretty confusing. More motivation to actually implement it for tunnelled migration :) But I won't put up a fight, so if you'd prefer a revert I'll ACK that. - Cole

Because it's a valid combination. p2p still uses a separate channel for qemu migration, so there's value in letting the user specify a manual migrate URI for overriding auto-port, or libvirt's FQDN lookup. What _isn't_ allowed is --migrateuri and TUNNELLED, since there is no separate migration channel. Disallow that instead --- tools/virsh-domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4fdf4ba..eb8688d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8306,15 +8306,15 @@ doMigrate(void *opaque) if ((flags & VIR_MIGRATE_PEER2PEER) || vshCommandOptBool(cmd, "direct")) { - /* For peer2peer migration or direct migration we only expect one URI - * a libvirt URI, or a hypervisor specific URI. */ - if (migrateuri != NULL) { + /* migrateuri doesn't make sense for tunnelled migration */ + if (flags & VIR_MIGRATE_TUNNELLED && migrateuri != NULL) { vshError(ctl, "%s", _("migrate: Unexpected migrateuri for peer2peer/direct migration")); goto out; } - if (virDomainMigrateToURI2(dom, desturi, NULL, xml, flags, dname, 0) == 0) + if (virDomainMigrateToURI2(dom, desturi, migrateuri, + xml, flags, dname, 0) == 0) ret = '0'; } else { /* For traditional live migration, connect to the destination host directly. */

On 05/28/2013 02:44 PM, Cole Robinson wrote: > By actually showing the Open() error to the user > --- > src/qemu/qemu_migration.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Does qemuDomainObjExitRemote(vm) risk clobbering the last error message? Since it potentially unrefs vm on the last reference, I'm wondering if we have to cache the last error at the right point in time instead of relying on current behavior. But obviously it worked in your testing as written.

> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 19b1236..9ac9be4 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -3472,7 +3472,8 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, > qemuDomainObjExitRemote(vm); > if (dconn == NULL) { > virReportError(VIR_ERR_OPERATION_FAILED, > - _("Failed to connect to remote libvirt URI %s"), dconnuri); > + _("Failed to connect to remote libvirt URI %s: %s"), > + dconnuri, virGetLastErrorMessage()); > virObjectUnref(cfg); > return -1; Another alternative is to just return -1 directly instead of doing a virReportError() that overwrites the original error, although I'm not sure if that loses some context that would help the user. Can you include a sample error message in your commit log that shows the improvement? If so, I'm inclined to ACK this.