Libguestfs - Feb 2014 - Libguestfs (1.22.6) driver/changes for mingw/win32

Hi,

I attached the changes I made to a vanilla libguestfs-1.22.6 in order to
make it work in mingw/win32.
Added is also the patch required to make QEMU compatible (add a command to
QMP that lists the supported devices (the regilat way you do it print it to
stderr, which is difficult to redirect in win32)).

This is done on behalf of Intel Corp.

Thanks,

Or (oberon in irc)

On Thu, Feb 13, 2014 at 04:54:09PM +0200, Or Goshen
wrote:
> Hi,
> 
> I attached the changes I made to a vanilla libguestfs-1.22.6 in order to
> make it work in mingw/win32.
> Added is also the patch required to make QEMU compatible (add a command to
> QMP that lists the supported devices (the regilat way you do it print it to
> stderr, which is difficult to redirect in win32)).
A couple of points first:

- You need to start using git.

- You need to develop against the git head, not the old 1.22 branch.

I took the tarball and turned it into a git commit, which I then
cherry picked on top of head.  (Except for 'bootstrap' which seems to
be a completely rewritten file -- so I created it as
'bootstrap.win32').

This is attached so you can take it from here.  As this was mostly an
automatic process -- git did the hard work, I just resolved a few
things that git could not resolve -- you need to check carefully that
this patch still works.

About the patch itself:

The patch looks fairly sensible.  There's some tidying up that could
be done, and it would be nice to confine more of the changes so they
don't affect the main code.  But there's nothing here that can't be
pushed upstream in some form, after a proper review and more testing.

The qemu change needs to separately be submitted to the qemu mailing
list, along with an explanation for why that is necessary.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)

On Mon, Feb 17, 2014 at 01:53:02PM +0000, Richard W.M. Jones
wrote:
> On Thu, Feb 13, 2014 at 04:54:09PM +0200, Or Goshen wrote:
> > Hi,
> > 
> > I attached the changes I made to a vanilla libguestfs-1.22.6 in order
to
> > make it work in mingw/win32.
> > Added is also the patch required to make QEMU compatible (add a
command to
> > QMP that lists the supported devices (the regilat way you do it print
it to
> > stderr, which is difficult to redirect in win32)).
> 
> A couple of points first:
> 
> - You need to start using git.
> 
> - You need to develop against the git head, not the old 1.22 branch.
> 
> I took the tarball and turned it into a git commit, which I then
> cherry picked on top of head.  (Except for 'bootstrap' which seems
to
> be a completely rewritten file -- so I created it as
> 'bootstrap.win32').
> 
> This is attached so you can take it from here.  As this was mostly an
> automatic process -- git did the hard work, I just resolved a few
> things that git could not resolve -- you need to check carefully that
> this patch still works.
> 
> About the patch itself:
> 
> The patch looks fairly sensible.  There's some tidying up that could
> be done, and it would be nice to confine more of the changes so they
> don't affect the main code.  But there's nothing here that
can't be
> pushed upstream in some form, after a proper review and more testing.
IMHO most of the diff that involves socket accept/select/read/write
and the Windows HANDLE <-> FD conversion looks really dubious and is
likely redundant. If you're using enough of the GNULIB modules, then
it should make all the WINSOCK awfulness go away such that normal
POSIX calls and poll/select stuff 'just works'.

The main missing socket functionality in Win32 that GNULIB doesn't
solve is a socketpair() impl, for which localhost bound IPv4 socket,
is normal.

GNULIB also solves the O_NONBLOCK vs FIONBIO problem too via
its 'set_nonblocking_flag' function.

IOW from here onwards:
> diff --git a/src/conn-socket.c b/src/conn-socket.c
> index fe3ca04..e21fde7 100644
> --- a/src/conn-socket.c
> +++ b/src/conn-socket.c
> @@ -20,16 +20,22 @@
>  
>  #include <config.h>
>  
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> +#include <io.h>
> +#include <Winsock2.h>
> +#else
>  #include <unistd.h>
>  #include <fcntl.h>
> -#include <errno.h>
>  #include <poll.h>
>  #include <sys/stat.h>
>  #include <sys/socket.h>
>  #include <sys/types.h>
> +#endif
> +
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <errno.h>
>  #include <assert.h>
>  
>  #include "guestfs.h"
> @@ -47,8 +53,12 @@ struct connection_socket {
>    int daemon_accept_sock;
>  };
>  
> +#define FD_TO_SOCKET(fd)   ((SOCKET) _get_osfhandle ((fd)))
> +#define SOCKET_TO_FD(fh)   (_open_osfhandle ((intptr_t) (fh), O_RDWR |
O_BINARY))
> +
>  static int handle_log_message (guestfs_h *g, struct connection_socket
*conn);
>  
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>  static int
>  accept_connection (guestfs_h *g, struct connection *connv)
>  {
> @@ -125,12 +135,75 @@ accept_connection (guestfs_h *g, struct connection
*connv)
>    /* Make sure the new socket is non-blocking. */
>    if (fcntl (conn->daemon_sock, F_SETFL, O_NONBLOCK) == -1) {
>      perrorf (g, "accept_connection: fcntl");
> -    return -1;
> +	return -1;
>    }
> -
>    return 1;
>  }
> +#else
> +static int accept_connection(guestfs_h *g, struct connection *connv) {
> +	struct connection_socket *conn = (struct connection_socket *) connv;
> +	SOCKET sock = INVALID_SOCKET;
>  
> +	if (conn->daemon_accept_sock == -1) {
> +		error(g, _("accept_connection called twice"));
> +		return -1;
> +	}
> +
> +	while (sock == INVALID_SOCKET) {
> +		int r;
> +		fd_set rfds;
> +
> +		FD_ZERO (&rfds);
> +
> +		FD_SET (FD_TO_SOCKET(conn->daemon_accept_sock), &rfds);
> +		if (conn->console_sock >= 0) {
> +		  FD_SET (FD_TO_SOCKET(conn->console_sock), &rfds);
> +		}
> +
> +		r = select(0, &rfds, NULL, NULL, NULL);
> +		if (r > 0) {
> +			if (FD_ISSET(FD_TO_SOCKET(conn->console_sock), &rfds)) {
> +			  r = handle_log_message (g, conn);
> +			  if (r <= 0)
> +				return r;
> +			}
> +
> +			if (FD_ISSET(FD_TO_SOCKET(conn->daemon_accept_sock), &rfds)) {
> +				sock = accept(FD_TO_SOCKET(conn->daemon_accept_sock), NULL, NULL);
> +				if (sock == INVALID_SOCKET) {
> +					if (errno == EINTR || errno == EAGAIN)
> +						continue;
> +					perrorf(g, "accept_connection: accept");
> +					return -1;
> +				}
> +			}
> +		} else if (r == SOCKET_ERROR) {
> +		  errno = WSAGetLastError();
> +		  if (errno == EINTR || errno == EAGAIN)
> +			continue;
> +		  perrorf (g, "accept_connection: select");
> +		  return -1;
> +		}
> +	}
> +
> +	/* Got a connection and accepted it, so update the connection's
> +	 * internal status.
> +	 */
> +	closesocket(FD_TO_SOCKET(conn->daemon_accept_sock));
> +	conn->daemon_accept_sock = -1;
> +	conn->daemon_sock = sock;
> +
> +	/* Make sure the new socket is non-blocking. */
> +	u_long iMode = 0;
> +	if (ioctlsocket(conn->daemon_sock, FIONBIO,	&iMode) ==
SOCKET_ERROR) {
> +		perrorf(g, "accept_connection: ioctlsocket");
> +		return -1;
> +	}
> +	return 1;
> +}
> +#endif
> +
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>  static ssize_t
>  read_data (guestfs_h *g, struct connection *connv, void *bufv, size_t len)
>  {
> @@ -205,7 +278,75 @@ read_data (guestfs_h *g, struct connection *connv,
void *bufv, size_t len)
>  
>    return original_len;
>  }
> +#else
> +static ssize_t
> +read_data (guestfs_h *g, struct connection *connv, void *bufv, size_t len)
> +{
> +  char *buf = bufv;
> +  struct connection_socket *conn = (struct connection_socket *) connv;
> +  size_t original_len = len;
>  
> +  if (conn->daemon_sock == -1) {
> +    error (g, _("read_data: socket not connected"));
> +    return -1;
> +  }
> +
> +  while (len > 0) {
> +    int r;
> +    fd_set rfds;
> +
> +    FD_ZERO (&rfds);
> +
> +    FD_SET (conn->daemon_sock, &rfds);
> +    if (conn->console_sock >= 0)
> +      FD_SET (FD_TO_SOCKET(conn->console_sock), &rfds);
> +
> +    r = select(0, &rfds, NULL, NULL, NULL);
> +    if (r > 0) {
> +    	if (FD_ISSET(FD_TO_SOCKET(conn->console_sock), &rfds)) {
> +    	  r = handle_log_message (g, conn);
> +		  if (r <= 0)
> +			return r;
> +    	}
> +
> +    	if (FD_ISSET(conn->daemon_sock, &rfds)) {
> +			int n = recv(conn->daemon_sock, buf, len, 0);
> +			if (n == -1) {
> +				if (errno == EINTR || errno == EAGAIN)
> +					continue;
> +				if (errno == ECONNRESET) /* essentially the same as EOF case */
> +					goto closed;
> +				perrorf(g, "read_data: read");
> +				return -1;
> +			}
> +			if (n == 0) {
> +				closed:
> +				/* Even though qemu has gone away, there could be more log
> +				 * messages in the console socket buffer in the kernel.  Read
> +				 * them out here.
> +				 */
> +				if (g->verbose && conn->console_sock >= 0) {
> +					while (handle_log_message(g, conn) == 1);
> +				}
> +				return 0;
> +			}
> +
> +			buf += n;
> +			len -= n;
> +    	}
> +    } else if (r == -1) {
> +      if (errno == EINTR || errno == EAGAIN)
> +        continue;
> +      perrorf (g, "read_data: select");
> +      return -1;
> +    }
> +  }
> +
> +  return original_len;
> +}
> +#endif
> +
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>  static int
>  can_read_data (guestfs_h *g, struct connection *connv)
>  {
> @@ -233,7 +374,40 @@ can_read_data (guestfs_h *g, struct connection *connv)
>  
>    return (fd.revents & POLLIN) != 0 ? 1 : 0;
>  }
> +#else
> +static int
> +can_read_data (guestfs_h *g, struct connection *connv)
> +{
> +  struct connection_socket *conn = (struct connection_socket *) connv;
> +  int r;
> +  fd_set rfds;
> +  //struct timeval tv0 = { .tv_sec = 0, .tv_usec = 0};
> +  const struct timeval timeout = {0, 0};
>  
> +
> +  if (conn->daemon_sock == -1) {
> +    error (g, _("can_read_data: socket not connected"));
> +    return -1;
> +  }
> +
> +  FD_ZERO (&rfds);
> +
> +  FD_SET (conn->daemon_sock, &rfds);
> +
> + again:
> +  r = select(0, &rfds, NULL, NULL, &timeout);
> +  if (r == -1) {
> +    if (errno == EINTR || errno == EAGAIN)
> +      goto again;
> +    perrorf (g, "can_read_data: select");
> +    return -1;
> +  }
> +
> +  return FD_ISSET(conn->daemon_sock, &rfds) ? 1 : 0;
> +}
> +#endif
> +
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>  static ssize_t
>  write_data (guestfs_h *g, struct connection *connv,
>              const void *bufv, size_t len)
> @@ -263,7 +437,8 @@ write_data (guestfs_h *g, struct connection *connv,
>        nfds++;
>      }
>  
> -    r = poll (fds, nfds, -1);
> +    r = 1;//poll (fds, nfds, -1);
> +    fds[0].revents = POLLOUT;
>      if (r == -1) {
>        if (errno == EINTR || errno == EAGAIN)
>          continue;
> @@ -297,6 +472,64 @@ write_data (guestfs_h *g, struct connection *connv,
>  
>    return original_len;
>  }
> +#else
> +static ssize_t
> +write_data (guestfs_h *g, struct connection *connv,
> +            const void *bufv, size_t len)
> +{
> +  const char *buf = bufv;
> +  struct connection_socket *conn = (struct connection_socket *) connv;
> +  size_t original_len = len;
> +
> +  if (conn->daemon_sock == -1) {
> +    error (g, _("write_data: socket not connected"));
> +    return -1;
> +  }
> +
> +  while (len > 0) {
> +    int r;
> +	fd_set rfds, wfds;
> +
> +	FD_ZERO (&rfds);
> +	FD_ZERO (&wfds);
> +
> +	FD_SET (conn->daemon_sock, &wfds);
> +	if (conn->console_sock >= 0)
> +	  FD_SET (FD_TO_SOCKET(conn->console_sock), &rfds);
> +
> +	r = select(0, &rfds, &wfds, NULL, NULL);
> +	if (r > 0) {
> +		if (FD_ISSET(FD_TO_SOCKET(conn->console_sock), &rfds)) {
> +		  r = handle_log_message (g, conn);
> +		  if (r <= 0)
> +			return r;
> +		}
> +
> +		if (FD_ISSET(conn->daemon_sock, &wfds)) {
> +			int n = send(conn->daemon_sock, buf, len, 0);
> +			if (n == -1) {
> +				if (errno == EINTR || errno == EAGAIN)
> +					continue;
> +				if (errno == EPIPE) /* Disconnected from guest (RHBZ#508713). */
> +					return 0;
> +				perrorf(g, "write_data: write");
> +				return -1;
> +			}
> +
> +			buf += n;
> +			len -= n;
> +		}
> +	} else if (r == -1) {
> +	  if (errno == EINTR || errno == EAGAIN)
> +		continue;
> +	  perrorf (g, "write_data: select");
> +	  return -1;
> +	}
> +  }
> +
> +  return original_len;
> +}
> +#endif
>  
>  /* This is called if conn->console_sock becomes ready to read while we
>   * are doing one of the connection operations above.  It reads and
> @@ -326,7 +559,9 @@ handle_log_message (guestfs_h *g,
>     *   based console (not yet implemented) we may be able to remove
>     *   this.  XXX"
>     */
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>    usleep (1000);
> +#endif
>  
>    n = read (conn->console_sock, buf, sizeof buf);
>    if (n == 0)
> @@ -366,6 +601,7 @@ handle_log_message (guestfs_h *g,
>    return 1;
>  }
>  
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>  static void
>  free_conn_socket (guestfs_h *g, struct connection *connv)
>  {
> @@ -380,6 +616,22 @@ free_conn_socket (guestfs_h *g, struct connection
*connv)
>  
>    free (conn);
>  }
> +#else
> +static void
> +free_conn_socket (guestfs_h *g, struct connection *connv)
> +{
> +  struct connection_socket *conn = (struct connection_socket *) connv;
> +
> +  if (conn->console_sock >= 0)
> +    closesocket (FD_TO_SOCKET(conn->console_sock));
> +  if (conn->daemon_sock >= 0)
> +    closesocket (conn->daemon_sock);
> +  if (conn->daemon_accept_sock >= 0)
> +    closesocket (FD_TO_SOCKET(conn->daemon_accept_sock));
> +
> +  free (conn);
> +}
> +#endif
>  
>  static struct connection_ops ops = {
>    .free_connection = free_conn_socket,
> @@ -407,13 +659,23 @@ guestfs___new_conn_socket_listening (guestfs_h *g,
>  
>    assert (daemon_accept_sock >= 0);
>  
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>    if (fcntl (daemon_accept_sock, F_SETFL, O_NONBLOCK) == -1) {
> +#else
> +  u_long iMode = 0;
> +  if (ioctlsocket(FD_TO_SOCKET(daemon_accept_sock), FIONBIO, &iMode)
== SOCKET_ERROR) {
> +#endif
>      perrorf (g, "new_conn_socket_listening: fcntl");
>      return NULL;
>    }
>  
>    if (console_sock >= 0) {
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>      if (fcntl (console_sock, F_SETFL, O_NONBLOCK) == -1) {
> +#else
> +    u_long iMode1 = 0;
> +    if (ioctlsocket(FD_TO_SOCKET(console_sock), FIONBIO, &iMode1) ==
SOCKET_ERROR) {
> +#endif
>        perrorf (g, "new_conn_socket_listening: fcntl");
>        return NULL;
>      }
> @@ -446,13 +708,23 @@ guestfs___new_conn_socket_connected (guestfs_h *g,
>  
>    assert (daemon_sock >= 0);
>  
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>    if (fcntl (daemon_sock, F_SETFL, O_NONBLOCK) == -1) {
> +#else
> +  u_long iMode = 0;
> +  if (ioctlsocket(FD_TO_SOCKET(daemon_sock), FIONBIO, &iMode) ==
SOCKET_ERROR) {
> +#endif
>      perrorf (g, "new_conn_socket_connected: fcntl");
>      return NULL;
>    }
>  
>    if (console_sock >= 0) {
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>      if (fcntl (console_sock, F_SETFL, O_NONBLOCK) == -1) {
> +#else
> +    u_long iMode1 = 0;
> +    if (ioctlsocket(FD_TO_SOCKET(console_sock), FIONBIO, &iMode1) ==
SOCKET_ERROR) {
> +#endif
>        perrorf (g, "new_conn_socket_connected: fcntl");
>        return NULL;
>      }
...to here should pretty much all be redundant.




The socketpair code in this file:
> diff --git a/src/launch-direct.c b/src/launch-direct.c
> index 964a507..1bc33c5 100644
> --- a/src/launch-direct.c
> +++ b/src/launch-direct.c
> @@ -257,6 +257,90 @@ debian_kvm_warning (guestfs_h *g)
>  }
>  
>  static int
> +get_listener_socket()
> +{
> +	union
> +	{
> +		struct sockaddr_in inaddr;
> +		struct sockaddr addr;
> +	} a;
> +
> +	int listener;
> +	int reuse = 1;
> +
> +	listener = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +	if (listener == -1)
> +		return -1;
> +
> +	memset(&a, 0, sizeof(a));
> +	a.inaddr.sin_family = AF_INET;
> +	a.inaddr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +	a.inaddr.sin_port = 0;
> +
> +	if (setsockopt(listener, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse,
(socklen_t)sizeof(reuse)) == -1)
> +		goto error1;
> +
> +	if (bind(listener, &a.addr, sizeof(a.inaddr)) == -1)
> +		goto error1;
> +
> +	return listener;
> +
> +error1:
> +	close(listener);
> +
> +	return -1;
> +}
> +
> +static int
> +mingw_socketpair(int socks[2])
> +{
> +	union
> +	{
> +		struct sockaddr_in inaddr;
> +		struct sockaddr addr;
> +	} a;
> +
> +	socklen_t addrlen;
> +	int listener = get_listener_socket();
> +	if (listener == -1)
> +		return -1;
> +
> +	memset(&a, 0, sizeof(a));
> +
> +	if (getsockname(listener, &a.addr, &addrlen) == -1)
> +		goto error1;
> +
> +	a.inaddr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +	a.inaddr.sin_family = AF_INET;
> +
> +	if (listen(listener, 1) == -1)
> +		goto error1;
> +
> +	socks[0] = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +	if (socks[0] == -1)
> +		goto error1;
> +
> +	if (connect(socks[0], &a.addr, sizeof(a.inaddr)) == -1)
> +		goto error2;
> +
> +	socks[1] = accept(listener, NULL, NULL);
> +	if (socks[1] == -1)
> +		goto error2;
> +
> +	close(listener);
> +
> +	return 0;
> +
> +error2:
> +	close(socks[0]);
> +
> +error1:
> +	close(listener);
> +
> +	return -1;
> +}

Seems to have been copied again in this file:
> diff --git a/src/launch-mingw.c b/src/launch-mingw.c
> new file mode 100644
> index 0000000..b944644
> --- /dev/null
> +++ b/src/launch-mingw.c
> +
> +#define QEMU_PORT 0
> +static int
> +get_listener_socket()
> +{
> +	union
> +	{
> +		struct sockaddr_in inaddr;
> +		struct sockaddr addr;
> +	} a;
> +
> +	int listener;
> +	int reuse = 1;
> +
> +	listener = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +	if (listener == -1)
> +		return -1;
> +
> +	memset(&a, 0, sizeof(a));
> +	a.inaddr.sin_family = AF_INET;
> +	a.inaddr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +	a.inaddr.sin_port = 0;//htons(QEMU_PORT);
> +
> +	if (setsockopt(listener, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse,
(socklen_t)sizeof(reuse)) == -1)
> +		goto error1;
> +
> +	if (bind(listener, &a.addr, sizeof(a.inaddr)) == -1)
> +		goto error1;
> +
> +	return listener;
> +
> +error1:
> +	close(listener);
> +
> +	return -1;
> +}
> +
> +static int
> +mingw_socketpair(int socks[2])
> +{
> +	union
> +	{
> +		struct sockaddr_in inaddr;
> +		struct sockaddr addr;
> +	} a;
> +
> +	socklen_t addrlen = sizeof a.addr;
> +	int listener;
> +	int reuse = 1;
> +
> +	listener = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +	if (listener == -1)
> +		return -1;
> +
> +	memset(&a, 0, sizeof(a));
> +	a.inaddr.sin_family = AF_INET;
> +	a.inaddr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +	a.inaddr.sin_port = 0;
> +
> +	if (setsockopt(listener, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse,
(socklen_t)sizeof(reuse)) == -1)
> +		goto error1;
> +
> +	if (bind(listener, &a.addr, sizeof(a.inaddr)) == -1)
> +		goto error1;
> +
> +	if (getsockname(listener, &a.addr, &addrlen) == -1)
> +		goto error1;
> +
> +	// port number for console to listen to
> +	int port = ntohs(a.inaddr.sin_port);
> +
> +	if (listen(listener, 1) == -1)
> +		goto error1;
> +
> +	a.inaddr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +	a.inaddr.sin_family = AF_INET;
> +
> +	socks[0] = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +	if (socks[0] == -1)
> +		goto error1;
> +
> +	if (connect(socks[0], &a.addr, sizeof(a.inaddr)) == -1)
> +		goto error2;
> +
> +	socks[1] = accept(listener, NULL, NULL);
> +	if (socks[1] == -1)
> +		goto error2;
> +
> +	close(listener);
> +
> +	return 0;
> +
> +error2:
> +	close(socks[0]);
> +
> +error1:
> +	close(listener);
> +
> +	return -1;
> +}
> +
> diff --git a/src/proto.c b/src/proto.c
> index 8001c8c..4744b37 100644
> --- a/src/proto.c
> +++ b/src/proto.c
> @@ -328,7 +326,11 @@ guestfs___send_file (guestfs_h *g, const char
*filename)
>  
>    g->user_cancel = 0;
>  
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>    fd = open (filename, O_RDONLY|O_CLOEXEC);
> +#else
> +  fd = _open (filename, _O_RDONLY|_O_BINARY|_O_NOINHERIT|_O_SEQUENTIAL);
> +#endif
>    if (fd == -1) {
>      perrorf (g, "open: %s", filename);
>      send_file_cancellation (g);
> @@ -339,7 +341,11 @@ guestfs___send_file (guestfs_h *g, const char
*filename)
>  
>    /* Send file in chunked encoding. */
>    while (!g->user_cancel) {
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>      r = read (fd, buf, sizeof buf);
> +#else
> +    r = _read (fd, buf, sizeof buf);
> +#endif
>      if (r == -1 && (errno == EINTR || errno == EAGAIN))
>        continue;
>      if (r <= 0) break;
> @@ -370,7 +376,11 @@ guestfs___send_file (guestfs_h *g, const char
*filename)
>    /* End of file, but before we send that, we need to close
>     * the file and check for errors.
>     */
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>    if (close (fd) == -1) {
> +#else
> +  if (_close (fd) == -1) {
> +#endif
>      perrorf (g, "close: %s", filename);
>      send_file_cancellation (g);
>      return -1;
> @@ -734,7 +744,11 @@ xwrite (int fd, const void *v_buf, size_t len)
>    int r;
>  
>    while (len > 0) {
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>      r = write (fd, buf, len);
> +#else
> +    r = _write (fd, buf, len);
> +#endif
>      if (r == -1)
>        return -1;
>  
> @@ -756,7 +770,11 @@ guestfs___recv_file (guestfs_h *g, const char
*filename)
>  
>    g->user_cancel = 0;
>  
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>    fd = open (filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY|O_CLOEXEC, 0666);
> +#else
> +  fd = _open (filename,
_O_WRONLY|_O_CREAT|_O_BINARY|_O_TRUNC|_O_NOINHERIT, 0666);
> +#endif
>    if (fd == -1) {
>      perrorf (g, "open: %s", filename);
>      goto cancel;
Conditionally calling the versions with _ seems pretty dubious too

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

Here is an initial review.
> diff --git a/src/actions-support.c b/src/actions-support.c
> index c3bd863..2c2d927 100644
> --- a/src/actions-support.c
> +++ b/src/actions-support.c
> @@ -81,7 +81,11 @@ guestfs___trace_open (struct trace_buffer *tb)
>  {
>    tb->buf = NULL;
>    tb->len = 0;
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
There are a lot of places which use this expression (or its negative).
You could simplify those by adding something like the following to
src/guestfs-internal-all.h:

  #define IS_WIN32 ((defined _WIN32 || defined __WIN32__) && !defined
__CYGWIN__)

and then replacing all #if conditions with:

  #if IS_WIN32
> diff --git a/src/appliance.c b/src/appliance.c
> index 3eaf635..d7dad0f 100644
> --- a/src/appliance.c
> +++ b/src/appliance.c
[...]

Large parts of src/appliance.c are individually conditionally removed.

That's fair enough, as I assume you are using the fixed appliance, but
I think it would be cleaner to rearrange the src/appliance.c code into
parts which are responsible for locating the appliance and parts which
are responsible for building the appliance.  Windows only need the
former, not the latter.  There are other platforms, eg. OS X, where we
don't want to build the appliance.

If you put these in separate files, then these can be conditionally
included at configure time, so in src/Makefile.am:

  libguestfs_la_SOURCES = ... appliance-find.c ...

  if CAN_BUILD_SUPERMIN_APPLIANCE
  libguestfs_la_SOURCES += appliance-build.c
  endif

(where CAN_BUILD_SUPERMIN_APPLIANCE is controlled in configure.ac, and
not defined on Windows or Mac OS X).
> @@ -907,7 +920,11 @@ find_path (guestfs_h *g,
>     * libguestfs < 1.5.4).
>     */
>    do {
> +#if (!defined _WIN32 && !defined __WIN32__) || defined __CYGWIN__
>      len = strcspn (pelem, ":");
> +#else
> +    len = strcspn (pelem, ";");
> +#endif
I improved on this by using autoconf which already defines
PATH_SEPARATOR, and pushed this hunk upstream, thanks.

https://github.com/libguestfs/libguestfs/commit/6b71b81a5f4b1bfbf9fb4efa576fe7e7ae2368a8
> diff --git a/src/command.c b/src/command.c
> index 02e5801..0b120f2 100644
> --- a/src/command.c
> +++ b/src/command.c
[...]
> @@ -129,6 +134,10 @@ struct command
>    int outfd;
>    struct buffering outbuf;
>  
> +  /* Supply input to be passed into stdin right after invacation */
"invocation"
> +  char *stdin_data;
> +  int infd;
This should be defended with #if IS_WIN32 ?
>    /* For programs that send output to stderr.  Hello qemu. */
>    bool stderr_to_stdout;
>  
> @@ -148,6 +157,7 @@ guestfs___new_command (guestfs_h *g)
>    cmd->close_files = true;
>    cmd->errorfd = -1;
>    cmd->outfd = -1;
> +  cmd->stdin_data = NULL;
You don't need to explicitly set fields to NULL/0 in the command
struct, since it is calloc'd already.  Just omit this line.
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> +static void
> +restore_file_desc(int orig_stdout, int orig_stderr, int orig_stdin)
As a point of style, we prefer that you use a single space between
function names and the open parenthesis '(' since it improves
readability.

I'm not going to review the WIN32 code.  Before applying any patches
however I would like to see if it at least compiles using our
mingw-gcc cross-compiler.
>  /* The loop which reads errors and output and directs it either
>   * to the log or to the stdout callback as appropriate.
> @@ -524,7 +671,7 @@ loop (struct command *cmd)
>  
>    while (nr_fds > 0) {
>      rset2 = rset;
> -    r = select (maxfd+1, &rset2, NULL, NULL, NULL);
> +    r = 1;//select (maxfd+1, &rset2, NULL, NULL, NULL);
Seems to be left over?
> diff --git a/src/conn-socket.c b/src/conn-socket.c
> index fe3ca04..e21fde7 100644
> --- a/src/conn-socket.c
> +++ b/src/conn-socket.c
> @@ -20,16 +20,22 @@
>  
>  #include <config.h>
>  
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> +#include <io.h>
> +#include <Winsock2.h>
> +#else
>  #include <unistd.h>
>  #include <fcntl.h>
> -#include <errno.h>
>  #include <poll.h>
>  #include <sys/stat.h>
>  #include <sys/socket.h>
>  #include <sys/types.h>
> +#endif
> +
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <errno.h>
Is there a reason to move the #includes around?
>  #include <assert.h>
>  
>  #include "guestfs.h"
> @@ -47,8 +53,12 @@ struct connection_socket {
>    int daemon_accept_sock;
>  };
>  
> +#define FD_TO_SOCKET(fd)   ((SOCKET) _get_osfhandle ((fd)))
> +#define SOCKET_TO_FD(fh)   (_open_osfhandle ((intptr_t) (fh), O_RDWR |
O_BINARY))
I think these are #if IS_WIN32 only.
>  static int handle_log_message (guestfs_h *g, struct connection_socket
*conn);
>  
A comment about the rest of src/conn-socket.c:

Most of this file has been conditionalized so each function appears
twice, once for POSIX and once for Win32.

It would be better IMHO to put the Win32 version into a separate file,
and have the Makefile.am include one or the other version depending on
whether we are compiling on POSIX or Win32.
> diff --git a/src/handle.c b/src/handle.c
> index 687f059..1010a97 100644
> --- a/src/handle.c
> +++ b/src/handle.c
> @@ -422,10 +422,10 @@ shutdown_backend (guestfs_h *g, int check_for_errors)
>      ret = -1;
>  
>    /* Close sockets. */
> -  if (g->conn) {
> +  /*if (g->conn) {
>      g->conn->ops->free_connection (g, g->conn);
>      g->conn = NULL;
> -  }
> +  }*/
This appears to be left over.  We certainly do not want to drop this
code on Linux, since it will cause a memory leak.
> diff --git a/src/inspect-fs-windows.c b/src/inspect-fs-windows.c
> index 20e4d7f..91c3372 100644
> --- a/src/inspect-fs-windows.c
> +++ b/src/inspect-fs-windows.c
> @@ -376,6 +376,9 @@ check_windows_software_registry (guestfs_h *g, struct
inspect_fs *fs)
>  static int
>  check_windows_system_registry (guestfs_h *g, struct inspect_fs *fs)
>  {
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> +  return -1;
> +#else
>    int r;
>    size_t len = strlen (fs->windows_systemroot) + 64;
>    char system[len];
> @@ -540,6 +543,7 @@ check_windows_system_registry (guestfs_h *g, struct
inspect_fs *fs)
>    guestfs_hivex_close (g);
>  
>    return ret;
> +#endif
Why is this conditionalized on Windows?
>  
>  /* Windows Registry HKLM\SYSTEM\MountedDevices uses a blob of data
> @@ -551,6 +555,9 @@ check_windows_system_registry (guestfs_h *g, struct
inspect_fs *fs)
>  static char *
>  map_registry_disk_blob (guestfs_h *g, const void *blob)
>  {
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> +  return NULL;
> +#else
>    CLEANUP_FREE_STRING_LIST char **devices = NULL;
>    CLEANUP_FREE_PARTITION_LIST struct guestfs_partition_list *partitions =
NULL;
>    size_t i, j, len;
> @@ -597,6 +604,7 @@ map_registry_disk_blob (guestfs_h *g, const void *blob)
>   found_partition:
>    /* Construct the full device name. */
>    return safe_asprintf (g, "%s%d", devices[i],
partitions->val[j].part_num);
> +#endif
>  }
Ditto, why did it need to be conditionalized?
>  /* NB: This function DOES NOT test for the existence of the file.  It
> diff --git a/src/launch-direct.c b/src/launch-direct.c
> index 964a507..1bc33c5 100644
> --- a/src/launch-direct.c
> +++ b/src/launch-direct.c
> @@ -257,6 +257,90 @@ debian_kvm_warning (guestfs_h *g)
>  }
>  
>  static int
> +get_listener_socket()
> +{
> +	union
We also prefer GNU-style indenting, not 8 characters.  If you use
emacs it will indent automatically in the correct style.
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> +  union
> +  {
> +    struct sockaddr_in inaddr;
> +    struct sockaddr addr;
> +  } a;
> +  socklen_t addrlen;
> +
> +  daemon_accept_sock = get_listener_socket();
> +  if (daemon_accept_sock == -1)
> +    goto cleanup0;
> +
> +  if (getsockname(daemon_accept_sock, &a.addr, &addrlen) == -1)
> +    goto cleanup0;
> +#else
>    snprintf (guestfsd_sock, sizeof guestfsd_sock,
"%s/guestfsd.sock", g->tmpdir);
>    unlink (guestfsd_sock);
>  
> @@ -333,6 +432,7 @@ launch_direct (guestfs_h *g, void *datav, const char
*arg)
>      perrorf (g, "bind");
>      goto cleanup0;
>    }
> +#endif
>  
>    if (listen (daemon_accept_sock, 1) == -1) {
>      perrorf (g, "listen");
> @@ -340,7 +440,11 @@ launch_direct (guestfs_h *g, void *datav, const char
*arg)
>    }
>  
>    if (!g->direct_mode) {
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> +	if (mingw_socketpair (sv) == -1) {
> +#else
>      if (socketpair (AF_LOCAL, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) {
> +#endif
>        perrorf (g, "socketpair");
>        goto cleanup0;
>      }
I feel there must be a cleaner way to write this instead of having the
conditional code.  Perhaps use autoconf's LIBOBJS feature?  But first
see my comment below about launch-mingw.c vs launch-direct.c.
> diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
> index 60213fd..1198bca 100644
> --- a/src/launch-libvirt.c
> +++ b/src/launch-libvirt.c
> @@ -25,7 +25,6 @@
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <limits.h>
> -#include <grp.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/wait.h>
> @@ -73,6 +72,8 @@
>  #if defined(HAVE_LIBVIRT) && \
>    LIBVIR_VERSION_NUMBER >= MIN_LIBVIRT_VERSION
>  
> +#include <grp.h>
> +
Why did the include of <grp.h> move?
> diff --git a/src/launch-mingw.c b/src/launch-mingw.c
> new file mode 100644
> index 0000000..b944644
> --- /dev/null
> +++ b/src/launch-mingw.c
Why are there changes to src/launch-direct.c, but also a new
'launch-mingw.c' file which seems to contain the same code?
> diff --git a/src/launch-unix.c b/src/launch-unix.c
> index 489a046..e968e6b 100644
> --- a/src/launch-unix.c
> +++ b/src/launch-unix.c
> @@ -23,7 +23,6 @@
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <sys/socket.h>
> -#include <sys/un.h>
>  
>  #include "guestfs.h"
>  #include "guestfs-internal.h"
> @@ -33,6 +32,15 @@
>  /* Alternate backend: instead of launching the appliance,
>   * connect to an existing unix socket.
>   */
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> +static int
> +launch_unix (guestfs_h *g, const char *sockpath)
> +{
> +	return -1;
> +}
> +
> +#else
> +#include <sys/un.h>
>  
>  static int
>  launch_unix (guestfs_h *g, void *datav, const char *sockpath)
If launch-unix.c doesn't work at all on Win32, then you should
conditionalize the whole file in Makefile.am.
> diff --git a/src/proto.c b/src/proto.c
> index 8001c8c..4744b37 100644
> --- a/src/proto.c
> +++ b/src/proto.c
> @@ -167,10 +167,9 @@ check_daemon_socket (guestfs_h *g)
>  
>    assert (g->conn); /* callers must check this */
>  
> - again:
> +again:
>    if (! g->conn->ops->can_read_data (g, g->conn))
>      return 1;
> -
>    n = g->conn->ops->read_data (g, g->conn, buf, 4);
>    if (n <= 0) /* 0 or -1 */
>      return n;
> @@ -183,7 +182,6 @@ check_daemon_socket (guestfs_h *g)
>    if (flag == GUESTFS_PROGRESS_FLAG) {
>      char buf[PROGRESS_MESSAGE_SIZE];
>      guestfs_progress message;
> -
More non-semantic whitespace changes.  Please remove these from future
revisions of this patch.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org