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

On Thu, Mar 17, 2016 at 15:58:50 +0300, Nikolay Shirokovskiy wrote: > > > 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. Yeah, I didn't meant port should be automatically allocated before we enter the loop. Just something like port = nbdPort; ... for (i = 0; i < vm->def->ndisks; i++) { ... if (!port && (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0 || qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0)) goto exit_monitor; ... } But you'd also need to remember whether you started the nbd server to make sure priv->nbdPort is not set if nbdPort was set but no server was needed. So after all your solution might probably be better, just reformat it a bit: if (nbdPort) port = nbdPort; else if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) goto exit_monitor; Jirka