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

[Michal Privoznik]

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