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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list