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