[Libguestfs] Libguestfs (1.22.6) driver/changes for mingw/win32
Richard W.M. Jones
rjones at redhat.com
Mon Feb 17 14:57:25 UTC 2014
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
More information about the Libguestfs
mailing list