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

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(a)virtuozzo.com&gt; >> --- >> >> 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.