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