[libvirt] [PATCH 00/21] Support NBD for tunnelled migration

[John Ferlan]

On 11/18/2015 01:12 PM, Pavel Boldin wrote:
> The provided patchset implements NBD disk migration over a tunnelled
> connection provided by libvirt.
> 

[...]

>  daemon/remote.c                  |  50 ++++
>  docs/apibuild.py                 |   1 +
>  docs/hvsupport.pl                |   1 +
>  include/libvirt/libvirt-domain.h |   3 +
>  src/driver-hypervisor.h          |   8 +
>  src/libvirt-domain.c             |  43 ++++
>  src/libvirt_internal.h           |   6 +
>  src/libvirt_private.syms         |   1 +
>  src/qemu/qemu_driver.c           |  24 ++
>  src/qemu/qemu_migration.c        | 495 +++++++++++++++++++++++++++++++++------
>  src/qemu/qemu_migration.h        |   6 +
>  src/qemu/qemu_monitor.c          |  12 +
>  src/qemu/qemu_monitor.h          |   2 +
>  src/qemu/qemu_monitor_json.c     |  35 +++
>  src/qemu/qemu_monitor_json.h     |   2 +
>  src/remote/remote_driver.c       |  91 +++++--
>  src/remote/remote_protocol.x     |  19 +-
>  src/remote_protocol-structs      |   8 +
>  src/security/virt-aa-helper.c    |   4 +-
>  19 files changed, 719 insertions(+), 92 deletions(-)
> 

Although not my area of expertise - figured I could give this series at
least a glance as I'm working my way through the list I had of unread
patches. Also I was only able to git am -3 the first 15 patches... after
that some other change gets in the way. You may need to fix up a few
things and repost. Maybe go with shorter series to at least make progress...

Some high level thoughts...

First off - obviously it's a larger series. You see 21 patches and know
you have to set aside the time for proper review...  Of course many are
short which is exactly what is "requested", still I assume the quantity
causes the "put this on my todo list" reaction - hence the delay in
anyone looking. Not an excuse for having something upstream for a bit
without review, but I hope it's a logical explanation...

Second - I note liberal usage of "unsigned char uuid[VIR_UUID_BUFLEN] in
function headers. I believe those should be replaced by "const unsigned
char *uuid" instead. IIRC this can lead to buffer overflow type issues
(think stack space for args).

Third - could this tunnel be possibly used more generically?  That is
this use is for NBD, but just the barebones indicate it's an extra
communication stream/channel. Would it be beneficial to pass more than a
remote_uuid. Perhaps remote resource name and/or uri?  A number of
migrate API's seem to use a cookie - is that something that would be
useful?  That's a finer technical detail that I hope can be worked out.

Hopefully someone with that finer and detailed technical knowledge of
the migration protocol will also jump in ;-)


John