[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Add migration APIs for libxl driver



On Fri, Mar 09, 2012 at 06:55:55PM +0800, Chunyan Liu wrote:
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d5fa64a..5dc29a0 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> +static int doParseURI(const char *uri, char **p_hostname, int *p_port)
> +{
> +    char *p, *hostname;
> +    int port_nr = 0;
> +
> +    if (uri == NULL)
> +        return -1;
> +
> +    /* URI passed is a string "hostname[:port]" */
> +    if ((p = strrchr(uri, ':')) != NULL) { /* "hostname:port" */
> +        int n;
> +
> +        if (virStrToLong_i(p+1, NULL, 10, &port_nr) < 0) {
> +            libxlError(VIR_ERR_INVALID_ARG,
> +                        _("Invalid port number"));
> +            return -1;
> +        }
> +
> +        /* Get the hostname. */
> +        n = p - uri; /* n = Length of hostname in bytes. */
> +        if (n <= 0) {
> +            libxlError(VIR_ERR_INVALID_ARG,
> +                       _("Hostname must be specified in the URI"));
> +            return -1;
> +        }
> +
> +        if (virAsprintf(&hostname, "%s", uri) < 0) {
> +            virReportOOMError();
> +            return -1;
> +        }
> +
> +        hostname[n] = '\0';
> +    }
> +    else {/* "hostname" (or IP address) */
> +        if (virAsprintf(&hostname, "%s", uri) < 0) {
> +            virReportOOMError();
> +            return -1;
> +        }
> +    }
> +    *p_hostname = hostname;
> +    *p_port = port_nr;
> +    return 0;
> +}

Lets not re-invent URI parsing code with wierd special cases for
base hostnames. Please just use virURIParse, since there is no
compatibility issue here.

> +static void doMigrateReceive(void *opaque)
> +{
> +    migrate_receive_args *data = opaque;
> +    virConnectPtr conn = data->conn;
> +    int sockfd = data->sockfd;
> +    virDomainObjPtr vm = data->vm;
> +    libxlDriverPrivatePtr driver = conn->privateData;
> +    int recv_fd;
> +    struct sockaddr_in new_addr;
> +    socklen_t socklen = sizeof(new_addr);
> +    int len;
> +
> +    do {
> +        recv_fd = accept(sockfd, (struct sockaddr *)&new_addr, &socklen);
> +    } while(recv_fd < 0 && errno == EINTR);

[snip]

> +    sockfd = socket(AF_INET, SOCK_STREAM, 0);
> +    if (sockfd == -1) {
> +        libxlError(VIR_ERR_OPERATION_FAILED,
> +                   _("Failed to create socket for incoming migration"));
> +        goto cleanup;
> +    }
> +
> +    memset(&addr, 0, sizeof(addr));
> +    addr.sin_family = AF_INET;
> +    addr.sin_port = htons(port);
> +    addr.sin_addr.s_addr = htonl(INADDR_ANY);
> +
> +    if (bind(sockfd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
> +        libxlError(VIR_ERR_OPERATION_FAILED,
> +                   _("Fail to bind port for incoming migration"));
> +        goto cleanup;
> +    }
> +
> +    if (listen(sockfd, MAXCONN_NUM) < 0){
> +        libxlError(VIR_ERR_OPERATION_FAILED,
> +                   _("Fail to listen to incoming migration"));
> +        goto cleanup;
> +    }
> +
> +    if (VIR_ALLOC(args) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }

[snip]

> +    memset(&hints, 0, sizeof(hints));
> +    hints.ai_family = AF_INET;
> +    hints.ai_socktype = SOCK_STREAM;
> +    if (getaddrinfo(hostname, servname, &hints, &res) || res == NULL) {
> +        libxlError(VIR_ERR_OPERATION_FAILED,
> +                   _("IP address lookup failed"));
> +        goto cleanup;
> +    }
> +
> +    if (connect(sockfd, res->ai_addr, res->ai_addrlen) < 0) {
> +        libxlError(VIR_ERR_OPERATION_FAILED,
> +                   _("Connect error"));
> +        goto cleanup;
> +    }

I know the QEMU code doesn't follow what I'm about to say, but I would
prefer it if all this socket code was re-written to use the internal
virNetSocketPtr APIs.  In particular this would result in correct
usage of getaddrinfo() which is lacking here.



> diff --git a/src/libxl/libxl_driver.h b/src/libxl/libxl_driver.h
> index 4632d33..ad8df1f 100644
> --- a/src/libxl/libxl_driver.h
> +++ b/src/libxl/libxl_driver.h
> @@ -21,9 +21,25 @@
>  /*---------------------------------------------------------------------------*/
> 
>  #ifndef LIBXL_DRIVER_H
> -# define LIBXL_DRIVER_H
> +#define LIBXL_DRIVER_H
> 
> -# include <config.h>
> +#include <config.h>
> +
> +#define LIBXL_MIGRATION_FLAGS                   \
> +    (VIR_MIGRATE_LIVE |                         \
> +     VIR_MIGRATE_UNDEFINE_SOURCE |              \
> +     VIR_MIGRATE_PAUSED)
> +
> +#define MAXCONN_NUM 10
> +#define LIBXL_MIGRATION_MIN_PORT 49512
> +#define LIBXL_MIGRATION_NUM_PORTS 64
> +#define LIBXL_MIGRATION_MAX_PORT                \
> +    (LIBXL_MIGRATION_MIN_PORT + LIBXL_MIGRATION_NUM_PORTS)
> +
> +static const char migrate_receiver_banner[]=
> +    "xl migration receiver ready, send binary domain data";
> +static const char migrate_receiver_ready[]=
> +    "domain received, ready to unpause";

It is better if these were in the .c file, or if they need to be
shared across multiple driver files, use the libxl_conf.h

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]