[PATCH 0/2] nbd: build qemu-nbd on Windows

[PATCH 0/2] nbd: build qemu-nbd on Windows

[Daniel P. Berrangé]

We are already building the NBD client and server on Windows when it is
used via the main system emulator binaries. This demonstrates there is
no fundamental blocker to buildig the qemu-nbd binary too.


In testing this I used:

   wine qemu-nbd.exe -t -p 9000 demo.img 

and

   wine qemu-img.exe info nbd:localhost:9000

In fact I tested the full matrix of native vs windows client and native
vs windows server.

I did notice that native qemu-img will sometimes hang when talking to
the windows qemu-nbd.exe binary, and the windows qemu-img will almost
always hang.

The hang happens in the "blk_unref" call in collect_image_info_list().
This suggests something related to the socket teardown / cleanup in
the NBD code.

While we should obviously investigate and fix that, I didn't consider
it a blocker for enabling build of qemu-nbd.exe, since we're already
building the same (buggy) NBD code in the system emulators on Windows.

Daniel P. Berrangé (2):
  block: add missing socket_init() calls to tools
  nbd: disable signals and forking on Windows builds

 meson.build |  7 ++-----
 qemu-img.c  |  2 ++
 qemu-io.c   |  2 ++
 qemu-nbd.c  | 11 ++++++++++-
 4 files changed, 16 insertions(+), 6 deletions(-)

-- 
2.26.2

[PATCH 1/2] block: add missing socket_init() calls to tools

[Daniel P. Berrangé]

Any tool that uses sockets needs to call socket_init() in order to work
on the Windows platform.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qemu-img.c | 2 ++
 qemu-io.c  | 2 ++
 qemu-nbd.c | 1 +
 3 files changed, 5 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 5308773811..eb2fc1f862 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -41,6 +41,7 @@
 #include "qemu/log.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
+#include "qemu/sockets.h"
 #include "qemu/units.h"
 #include "qom/object_interfaces.h"
 #include "sysemu/block-backend.h"
@@ -5410,6 +5411,7 @@ int main(int argc, char **argv)
     signal(SIGPIPE, SIG_IGN);
 #endif
 
+    socket_init();
     error_init(argv[0]);
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_exec_dir(argv[0]);
diff --git a/qemu-io.c b/qemu-io.c
index 3adc5a7d0d..7cc832b3d6 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -25,6 +25,7 @@
 #include "qemu/config-file.h"
 #include "qemu/readline.h"
 #include "qemu/log.h"
+#include "qemu/sockets.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qdict.h"
 #include "qom/object_interfaces.h"
@@ -542,6 +543,7 @@ int main(int argc, char **argv)
     signal(SIGPIPE, SIG_IGN);
 #endif
 
+    socket_init();
     error_init(argv[0]);
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_exec_dir(argv[0]);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index d2657b8db5..b102874f0f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -599,6 +599,7 @@ int main(int argc, char **argv)
     signal(SIGPIPE, SIG_IGN);
 #endif
 
+    socket_init();
     error_init(argv[0]);
     module_call_init(MODULE_INIT_TRACE);
     qcrypto_init(&error_fatal);
-- 
2.26.2

Re: [PATCH 1/2] block: add missing socket_init() calls to tools

[Eric Blake]

On 8/24/20 12:02 PM, Daniel P. Berrangé wrote:
> Any tool that uses sockets needs to call socket_init() in order to work
> on the Windows platform.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   qemu-img.c | 2 ++
>   qemu-io.c  | 2 ++
>   qemu-nbd.c | 1 +
>   3 files changed, 5 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH 2/2] nbd: disable signals and forking on Windows builds

[Daniel P. Berrangé]

Disabling these parts are sufficient to get the qemu-nbd program
compiling in a Windows build.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 meson.build |  7 ++-----
 qemu-nbd.c  | 10 +++++++++-
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/meson.build b/meson.build
index df5bf728b5..1071871605 100644
--- a/meson.build
+++ b/meson.build
@@ -1074,12 +1074,9 @@ if have_tools
              dependencies: [authz, block, crypto, io, qom, qemuutil], install: true)
   qemu_io = executable('qemu-io', files('qemu-io.c'),
              dependencies: [block, qemuutil], install: true)
-  qemu_block_tools += [qemu_img, qemu_io]
-  if targetos == 'linux' or targetos == 'sunos' or targetos.endswith('bsd')
-    qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
+  qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
                dependencies: [block, qemuutil], install: true)
-    qemu_block_tools += [qemu_nbd]
-  endif
+  qemu_block_tools += [qemu_img, qemu_io, qemu_nbd]
 
   subdir('storage-daemon')
   subdir('contrib/rdmacm-mux')
diff --git a/qemu-nbd.c b/qemu-nbd.c
index b102874f0f..c6fd6524d3 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -155,12 +155,13 @@ QEMU_COPYRIGHT "\n"
     , name);
 }
 
+#ifndef WIN32
 static void termsig_handler(int signum)
 {
     atomic_cmpxchg(&state, RUNNING, TERMINATE);
     qemu_notify_event();
 }
-
+#endif
 
 static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
                                 const char *hostname)
@@ -587,6 +588,7 @@ int main(int argc, char **argv)
     unsigned socket_activation;
     const char *pid_file_name = NULL;
 
+#ifndef WIN32
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
      */
@@ -594,6 +596,7 @@ int main(int argc, char **argv)
     memset(&sa_sigterm, 0, sizeof(sa_sigterm));
     sa_sigterm.sa_handler = termsig_handler;
     sigaction(SIGTERM, &sa_sigterm, NULL);
+#endif
 
 #ifdef CONFIG_POSIX
     signal(SIGPIPE, SIG_IGN);
@@ -896,6 +899,7 @@ int main(int argc, char **argv)
 #endif
 
     if ((device && !verbose) || fork_process) {
+#ifndef WIN32
         int stderr_fd[2];
         pid_t pid;
         int ret;
@@ -959,6 +963,10 @@ int main(int argc, char **argv)
              */
             exit(errors);
         }
+#else /* WIN32 */
+        error_report("Unable to fork into background on Windows hosts");
+        exit(EXIT_FAILURE);
+#endif /* WIN32 */
     }
 
     if (device != NULL && sockpath == NULL) {
-- 
2.26.2

Re: [PATCH 2/2] nbd: disable signals and forking on Windows builds

[Eric Blake]

On 8/24/20 12:02 PM, Daniel P. Berrangé wrote:
> Disabling these parts are sufficient to get the qemu-nbd program
> compiling in a Windows build.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   meson.build |  7 ++-----
>   qemu-nbd.c  | 10 +++++++++-
>   2 files changed, 11 insertions(+), 6 deletions(-)

Feels a bit hacky at what it supports, but certainly better than nothing ;)

> 
> diff --git a/meson.build b/meson.build
> index df5bf728b5..1071871605 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1074,12 +1074,9 @@ if have_tools
>                dependencies: [authz, block, crypto, io, qom, qemuutil], install: true)
>     qemu_io = executable('qemu-io', files('qemu-io.c'),
>                dependencies: [block, qemuutil], install: true)
> -  qemu_block_tools += [qemu_img, qemu_io]
> -  if targetos == 'linux' or targetos == 'sunos' or targetos.endswith('bsd')
> -    qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
> +  qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
>                  dependencies: [block, qemuutil], install: true)

Conflicts with this patch:
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg05546.html

but this one gets rid of the need for that one.

> -    qemu_block_tools += [qemu_nbd]
> -  endif
> +  qemu_block_tools += [qemu_img, qemu_io, qemu_nbd]
>   
>     subdir('storage-daemon')
>     subdir('contrib/rdmacm-mux')
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index b102874f0f..c6fd6524d3 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -155,12 +155,13 @@ QEMU_COPYRIGHT "\n"
>       , name);
>   }
>   
> +#ifndef WIN32
>   static void termsig_handler(int signum)
>   {
>       atomic_cmpxchg(&state, RUNNING, TERMINATE);
>       qemu_notify_event();
>   }
> -
> +#endif

How does one terminate a long-running server on Windows if there is no 
SIGTERM handler?  I guess Ctrl-C does something, but without the state 
notification from a signal handler, you are getting less-clean 
shutdowns, which may explain the hangs you were seeing in testing?  But 
incremental progress is fine, and I see no reason to not take this patch 
as-is.

Reviewed-by: Eric Blake <eblake@redhat.com>

I'm happy to queue this series through my NBD tree.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] nbd: disable signals and forking on Windows builds

[Daniel P. Berrangé]

On Mon, Aug 24, 2020 at 12:12:53PM -0500, Eric Blake wrote:
> On 8/24/20 12:02 PM, Daniel P. Berrangé wrote:
> > Disabling these parts are sufficient to get the qemu-nbd program
> > compiling in a Windows build.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   meson.build |  7 ++-----
> >   qemu-nbd.c  | 10 +++++++++-
> >   2 files changed, 11 insertions(+), 6 deletions(-)
> 
> Feels a bit hacky at what it supports, but certainly better than nothing ;)
> 
> > 
> > diff --git a/meson.build b/meson.build
> > index df5bf728b5..1071871605 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1074,12 +1074,9 @@ if have_tools
> >                dependencies: [authz, block, crypto, io, qom, qemuutil], install: true)
> >     qemu_io = executable('qemu-io', files('qemu-io.c'),
> >                dependencies: [block, qemuutil], install: true)
> > -  qemu_block_tools += [qemu_img, qemu_io]
> > -  if targetos == 'linux' or targetos == 'sunos' or targetos.endswith('bsd')
> > -    qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
> > +  qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
> >                  dependencies: [block, qemuutil], install: true)
> 
> Conflicts with this patch:
> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg05546.html
> 
> but this one gets rid of the need for that one.
> 
> > -    qemu_block_tools += [qemu_nbd]
> > -  endif
> > +  qemu_block_tools += [qemu_img, qemu_io, qemu_nbd]
> >     subdir('storage-daemon')
> >     subdir('contrib/rdmacm-mux')
> > diff --git a/qemu-nbd.c b/qemu-nbd.c
> > index b102874f0f..c6fd6524d3 100644
> > --- a/qemu-nbd.c
> > +++ b/qemu-nbd.c
> > @@ -155,12 +155,13 @@ QEMU_COPYRIGHT "\n"
> >       , name);
> >   }
> > +#ifndef WIN32
> >   static void termsig_handler(int signum)
> >   {
> >       atomic_cmpxchg(&state, RUNNING, TERMINATE);
> >       qemu_notify_event();
> >   }
> > -
> > +#endif
> 
> How does one terminate a long-running server on Windows if there is no
> SIGTERM handler?  I guess Ctrl-C does something, but without the state
> notification from a signal handler, you are getting less-clean shutdowns,
> which may explain the hangs you were seeing in testing?  But incremental
> progress is fine, and I see no reason to not take this patch as-is.

The hangs occurred even with windows client/ native server, just less
frequently so don't think it is related.

Re-reading the code I notice this SIGTERM handler is only there for
the NBD device client thread, so we should skip it if that is not
installed.

Ctrl-C does SIGINT, so that's unrelated to the SIGTERM handler.

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I'm happy to queue this series through my NBD tree.

I'll post a v2

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] nbd: disable signals and forking on Windows builds

[Thomas Huth]

On 24/08/2020 19.02, Daniel P. Berrangé wrote:
> Disabling these parts are sufficient to get the qemu-nbd program
> compiling in a Windows build.

Maybe add:
"This also enables compilation of qemu-nbd on macOS again (which got 
disabled by accident during the conversion to the meson build system)"

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   meson.build |  7 ++-----
>   qemu-nbd.c  | 10 +++++++++-
>   2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index df5bf728b5..1071871605 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1074,12 +1074,9 @@ if have_tools
>                dependencies: [authz, block, crypto, io, qom, qemuutil], install: true)
>     qemu_io = executable('qemu-io', files('qemu-io.c'),
>                dependencies: [block, qemuutil], install: true)
> -  qemu_block_tools += [qemu_img, qemu_io]
> -  if targetos == 'linux' or targetos == 'sunos' or targetos.endswith('bsd')
> -    qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
> +  qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
>                  dependencies: [block, qemuutil], install: true)
> -    qemu_block_tools += [qemu_nbd]
> -  endif
> +  qemu_block_tools += [qemu_img, qemu_io, qemu_nbd]
>   
>     subdir('storage-daemon')
>     subdir('contrib/rdmacm-mux')
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index b102874f0f..c6fd6524d3 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -155,12 +155,13 @@ QEMU_COPYRIGHT "\n"
>       , name);
>   }
>   
> +#ifndef WIN32
>   static void termsig_handler(int signum)
>   {
>       atomic_cmpxchg(&state, RUNNING, TERMINATE);
>       qemu_notify_event();
>   }
> -
> +#endif
>   
>   static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
>                                   const char *hostname)
> @@ -587,6 +588,7 @@ int main(int argc, char **argv)
>       unsigned socket_activation;
>       const char *pid_file_name = NULL;
>   
> +#ifndef WIN32
>       /* The client thread uses SIGTERM to interrupt the server.  A signal
>        * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
>        */
> @@ -594,6 +596,7 @@ int main(int argc, char **argv)
>       memset(&sa_sigterm, 0, sizeof(sa_sigterm));
>       sa_sigterm.sa_handler = termsig_handler;
>       sigaction(SIGTERM, &sa_sigterm, NULL);
> +#endif
>   
>   #ifdef CONFIG_POSIX

I wonder why the CONFIG_POSIX does not simply start earlier here ... I 
think you could replace your #ifndef WIN32 with #ifdef CONFIG_POSIX that 
way?

>       signal(SIGPIPE, SIG_IGN);
> @@ -896,6 +899,7 @@ int main(int argc, char **argv)
>   #endif
>   
>       if ((device && !verbose) || fork_process) {
> +#ifndef WIN32
>           int stderr_fd[2];
>           pid_t pid;
>           int ret;
> @@ -959,6 +963,10 @@ int main(int argc, char **argv)
>                */
>               exit(errors);
>           }
> +#else /* WIN32 */
> +        error_report("Unable to fork into background on Windows hosts");
> +        exit(EXIT_FAILURE);
> +#endif /* WIN32 */
>       }
>   
>       if (device != NULL && sockpath == NULL) {
> 

  Thomas

Re: [PATCH 0/2] nbd: build qemu-nbd on Windows

[Daniel P. Berrangé]

On Mon, Aug 24, 2020 at 06:02:16PM +0100, Daniel P. Berrangé wrote:
> We are already building the NBD client and server on Windows when it is
> used via the main system emulator binaries. This demonstrates there is
> no fundamental blocker to buildig the qemu-nbd binary too.
> 
> 
> In testing this I used:
> 
>    wine qemu-nbd.exe -t -p 9000 demo.img 
> 
> and
> 
>    wine qemu-img.exe info nbd:localhost:9000
> 
> In fact I tested the full matrix of native vs windows client and native
> vs windows server.
> 
> I did notice that native qemu-img will sometimes hang when talking to
> the windows qemu-nbd.exe binary, and the windows qemu-img will almost
> always hang.
> 
> The hang happens in the "blk_unref" call in collect_image_info_list().
> This suggests something related to the socket teardown / cleanup in
> the NBD code.
> 
> While we should obviously investigate and fix that, I didn't consider
> it a blocker for enabling build of qemu-nbd.exe, since we're already
> building the same (buggy) NBD code in the system emulators on Windows.

After more debugging here's where it is getting stuck...

The main NBD client coroutine in qemu-img.exe is getting stuck in
nbd_read_eof() call where it has done qio_channel_readv() and got
QIO_CHANNEL_ERR_BLOCK. It has thus run qio_channel_yield(G_IO_IN)
and is waiting for that to return control, where upon it will exit
on EOF

Meanwhile the main qemu-img thread is in nbd_teardown_connection
having run qio_channel_shutdown(BOTH), and is now stuck forever in
BDRV_POLL_WHILE(bs, s->connection_co); waiting for the main NBD
coroutine to exit.


On native builds, we'll get G_IO_IN|G_IO_HUP in the coroutine after
calling the qio_channel_shutdown() in the main thread, and thus
the coroutine exits.

On windows builds running under Wine this doesn't seem to happen.
If I strace the wine program it does happen. IOW there's is some
kind of race condition wrt socket shutdown in QEMU when run in
Wine.

On windows builds running Windows Server 2008 r2, it appears to
work correctly. Maybe this is just luck, or maybe the bug really
is only affecting Wine. 

I don't intend to investigate this hang any more though, given
that it doesn't seem to reproduce in native Windows


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|