[libvirt] [PATCH 2/3] qemu: migration: error if tunnelled + storage specified

[Cole Robinson]

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