[libvirt] [PATCH v2] migration: add option to set target ndb server port

[Nikolay Shirokovskiy]

On 16.03.2016 18:59, Jiri Denemark wrote:
> On Tue, Feb 16, 2016 at 10:31:50 +0300, Nikolay Shirokovskiy wrote:
>> Current libvirt + qemu pair lacks secure migrations in case of
>> VMs with non-shared disks. The only option to migrate securely
>> natively is to use tunneled mode and some kind of secure
>> destination URI. But tunelled mode does not support non-shared
>> disks.
>>
>> The other way to make migration secure is to organize a tunnel
>> by external means. This is possible in case of shared disks
>> migration thru use of proper combination of destination URI,
>> migration URI and VIR_MIGRATE_PARAM_LISTEN_ADDRESS migration
>> param. But again this is not possible in case of non shared disks
>> migration as we have no option to control target nbd server port.
>> But fixing this much more simplier that supporting non-shared
>> disks in tunneled mode.
>>
>> So this patch adds option to set target ndb port.
>>
>> Finally all qemu migration connections will be secured AFAIK but
>> even in this case this patch could be convinient if one wants
>> all migration traffic be put in a single connection.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>> ---
>>
>>  include/libvirt/libvirt-domain.h | 10 +++++
>>  src/qemu/qemu_driver.c           | 25 ++++++++----
>>  src/qemu/qemu_migration.c        | 85 ++++++++++++++++++++++++++++------------
>>  src/qemu/qemu_migration.h        |  3 ++
>>  tools/virsh-domain.c             | 12 ++++++
>>  tools/virsh.pod                  |  5 ++-
>>  6 files changed, 106 insertions(+), 34 deletions(-)

...

>> @@ -1733,10 +1740,15 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
>>                                             QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
>>              goto cleanup;
>>  
>> -        if (!port &&
>> -            ((virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) ||
>> -             (qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0))) {
>> -            goto exit_monitor;
>> +        if (port == 0) {
>> +            if (nbdPort)
>> +                port = nbdPort;
>> +            else
>> +                if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
>> +                    goto exit_monitor;
>> +
>> +            if (qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0)
>> +                goto exit_monitor;
> 
> If you initialize port = nbdPort at the beginning, you can just drop
> this change.
> 

Thank you for reviewing. 

I'm about to resend this patch (know series) and have 
only argue about this place. I can't just set port at the beginning. In this
case if nbdPort is specified nbd server will not be started. I hesitating
on how to change this place with less nesting (which looks like the problem)
and finally decided to split this cycle. Basically we want to delay starting
ndb server until we found we have some disks to migrate. Let's check 
this condition separately. In this case we could move logic of starting
nbd server out of the cycle. After that port acquring and nbd starting logic could
be written sequentially without using of extra flags of extra nesting.