On 29.05.2013 21:37, Cole Robinson wrote:
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
Mmm. Okay, I think we both have point there. So I can live with the code as is. After all, it will enforce me to finish the NBD storage migration for tunelled migration :) Michal