Re: [Xen-devel] [libvirt] [PATCH V5] libxl: add migration support

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

On Tue, Apr 29, 2014 at 04:46:50PM -0600, Jim Fehlig wrote:
> This patch adds initial migration support to the libxl driver,
> using the VIR_DRV_FEATURE_MIGRATION_PARAMS family of migration
> functions.
> 
> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
> 
> V4 here
>   https://www.redhat.com/archives/libvir-list/2014-April/msg01070.html
> 
> In V5:
> Use libvirt's V3 migration protocol for handshake and control instead
> of duplicating that in the libxl driver.
> 
>  po/POTFILES.in              |   1 +
>  src/Makefile.am             |   3 +-
>  src/libxl/libxl_conf.h      |   6 +
>  src/libxl/libxl_domain.h    |   1 +
>  src/libxl/libxl_driver.c    | 235 +++++++++++++++++++
>  src/libxl/libxl_migration.c | 544 
> ++++++++++++++++++++++++++++++++++++++++++++
>  src/libxl/libxl_migration.h |  78 +++++++
>  7 files changed, 867 insertions(+), 1 deletion(-)

Overall I like this version a lot better than the previous due
to the simplicity overall.


> +static void
> +libxlDoMigrateReceive(virNetSocketPtr sock,
> +                      int events ATTRIBUTE_UNUSED,
> +                      void *opaque)
> +{
> +    libxlMigrateReceiveArgs *data = opaque;
> +    virConnectPtr conn = data->conn;
> +    virDomainObjPtr vm = data->vm;
> +    virNetSocketPtr *socks = data->socks;
> +    size_t nsocks = data->nsocks;
> +    libxlDriverPrivatePtr driver = conn->privateData;
> +    virNetSocketPtr client_sock;
> +    int recvfd;
> +    size_t i;
> +    int ret;
> +
> +    virNetSocketAccept(sock, &client_sock);
> +    if (client_sock == NULL) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("Fail to accept migration connection"));
> +        goto cleanup;
> +    }
> +    VIR_DEBUG("Accepted migration connection\n");
> +    recvfd = virNetSocketDupFD(client_sock, true);
> +    virObjectUnref(client_sock);
> +
> +    virObjectLock(vm);
> +    ret = libxlDomainStart(driver, vm, false, recvfd);
> +    virObjectUnlock(vm);
> +
> +    if (ret < 0 && !vm->persistent)
> +        virDomainObjListRemove(driver->domains, vm);
> +
> + cleanup:
> +    /* Remove all listen socks from event handler, and close them. */
> +    for (i = 0; i < nsocks; i++) {
> +        virNetSocketUpdateIOCallback(socks[i], 0);
> +        virNetSocketRemoveIOCallback(socks[i]);
> +        virNetSocketClose(socks[i]);
> +        virObjectUnref(socks[i]);
> +    }
> +    VIR_FREE(socks);

This free'ing of socks seems unsafe, since libxlDoMigrateReceive
can be invoked once for each socket, and thus get a double-free

> +int
> +libxlDomainMigrationPrepare(virConnectPtr dconn,
> +                            virDomainDefPtr def,
> +                            const char *uri_in,
> +                            char **uri_out)
> +{

...

> +    snprintf(portstr, sizeof(portstr), "%d", port);
> +
> +    if (virNetSocketNewListenTCP(hostname, portstr, &socks, &nsocks) < 0) {
> +        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                       _("Fail to create socket for incoming migration"));
> +        goto cleanup;
> +    }
> +
> +    if (VIR_ALLOC(args) < 0)
> +        goto cleanup;

I'm not seeing what is responsible for freeing 'args'. This
function doesn't do it, and the libxlDoMigrateReceive callback
can't do it, since you have multiple callbacks passed the
same 'args' variable without ref-counting, so the callback
cannot tell if it is unused surely ?

> +
> +    args->conn = dconn;
> +    args->vm = vm;
> +    args->socks = socks;
> +    args->nsocks = nsocks;
> +
> +    for (i = 0; i < nsocks; i++) {
> +        if (virNetSocketSetBlocking(socks[i], true) < 0)
> +             continue;
> +
> +        if (virNetSocketListen(socks[i], 1) < 0)
> +            continue;
> +
> +        if (virNetSocketAddIOCallback(socks[i],
> +                                      0,
> +                                      libxlDoMigrateReceive,
> +                                      args,
> +                                      NULL) < 0) {
> +            continue;
> +        }
> +
> +        virNetSocketUpdateIOCallback(socks[i], VIR_EVENT_HANDLE_READABLE);

Any reason not to just pass VIR_EVENT_HANDLE_READABLE to the
AddIOCallback fnuction instead of '0' ?

> +        nsocks_listen++;
> +    }
> +
> +    if (!nsocks_listen)
> +        goto cleanup;
> +
> +    ret = 0;
> +    goto done;
> +
> + cleanup:
> +    for (i = 0; i < nsocks; i++) {
> +        virNetSocketClose(socks[i]);
> +        virObjectUnref(socks[i]);
> +    }
> +    VIR_FREE(socks);
> +
> + done:
> +    virURIFree(uri);
> +    if (vm)
> +        virObjectUnlock(vm);
> +    return ret;
> +}



Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel