* [PATCH 0/2] nbd: build qemu-nbd on Windows @ 2020-08-24 17:02 Daniel P. Berrangé 2020-08-24 17:02 ` [PATCH 1/2] block: add missing socket_init() calls to tools Daniel P. Berrangé ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Daniel P. Berrangé @ 2020-08-24 17:02 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block, Max Reitz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] block: add missing socket_init() calls to tools 2020-08-24 17:02 [PATCH 0/2] nbd: build qemu-nbd on Windows Daniel P. Berrangé @ 2020-08-24 17:02 ` Daniel P. Berrangé 2020-08-24 17:08 ` Eric Blake 2020-08-24 17:02 ` [PATCH 2/2] nbd: disable signals and forking on Windows builds Daniel P. Berrangé 2020-08-25 14:55 ` [PATCH 0/2] nbd: build qemu-nbd on Windows Daniel P. Berrangé 2 siblings, 1 reply; 8+ messages in thread From: Daniel P. Berrangé @ 2020-08-24 17:02 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block, Max Reitz 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] block: add missing socket_init() calls to tools 2020-08-24 17:02 ` [PATCH 1/2] block: add missing socket_init() calls to tools Daniel P. Berrangé @ 2020-08-24 17:08 ` Eric Blake 0 siblings, 0 replies; 8+ messages in thread From: Eric Blake @ 2020-08-24 17:08 UTC (permalink / raw) To: Daniel P. Berrangé, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] nbd: disable signals and forking on Windows builds 2020-08-24 17:02 [PATCH 0/2] nbd: build qemu-nbd on Windows Daniel P. Berrangé 2020-08-24 17:02 ` [PATCH 1/2] block: add missing socket_init() calls to tools Daniel P. Berrangé @ 2020-08-24 17:02 ` Daniel P. Berrangé 2020-08-24 17:12 ` Eric Blake 2020-08-25 5:35 ` Thomas Huth 2020-08-25 14:55 ` [PATCH 0/2] nbd: build qemu-nbd on Windows Daniel P. Berrangé 2 siblings, 2 replies; 8+ messages in thread From: Daniel P. Berrangé @ 2020-08-24 17:02 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block, Max Reitz 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] nbd: disable signals and forking on Windows builds 2020-08-24 17:02 ` [PATCH 2/2] nbd: disable signals and forking on Windows builds Daniel P. Berrangé @ 2020-08-24 17:12 ` Eric Blake 2020-08-25 10:35 ` Daniel P. Berrangé 2020-08-25 5:35 ` Thomas Huth 1 sibling, 1 reply; 8+ messages in thread From: Eric Blake @ 2020-08-24 17:12 UTC (permalink / raw) To: Daniel P. Berrangé, qemu-devel Cc: Kevin Wolf, Thomas Huth, qemu-block, Max Reitz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] nbd: disable signals and forking on Windows builds 2020-08-24 17:12 ` Eric Blake @ 2020-08-25 10:35 ` Daniel P. Berrangé 0 siblings, 0 replies; 8+ messages in thread From: Daniel P. Berrangé @ 2020-08-25 10:35 UTC (permalink / raw) To: Eric Blake; +Cc: Kevin Wolf, Thomas Huth, qemu-devel, qemu-block, Max Reitz 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 :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] nbd: disable signals and forking on Windows builds 2020-08-24 17:02 ` [PATCH 2/2] nbd: disable signals and forking on Windows builds Daniel P. Berrangé 2020-08-24 17:12 ` Eric Blake @ 2020-08-25 5:35 ` Thomas Huth 1 sibling, 0 replies; 8+ messages in thread From: Thomas Huth @ 2020-08-25 5:35 UTC (permalink / raw) To: Daniel P. Berrangé, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] nbd: build qemu-nbd on Windows 2020-08-24 17:02 [PATCH 0/2] nbd: build qemu-nbd on Windows Daniel P. Berrangé 2020-08-24 17:02 ` [PATCH 1/2] block: add missing socket_init() calls to tools Daniel P. Berrangé 2020-08-24 17:02 ` [PATCH 2/2] nbd: disable signals and forking on Windows builds Daniel P. Berrangé @ 2020-08-25 14:55 ` Daniel P. Berrangé 2 siblings, 0 replies; 8+ messages in thread From: Daniel P. Berrangé @ 2020-08-25 14:55 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Paolo Bonzini, qemu-block, Max Reitz 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 :| ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-25 14:56 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-24 17:02 [PATCH 0/2] nbd: build qemu-nbd on Windows Daniel P. Berrangé 2020-08-24 17:02 ` [PATCH 1/2] block: add missing socket_init() calls to tools Daniel P. Berrangé 2020-08-24 17:08 ` Eric Blake 2020-08-24 17:02 ` [PATCH 2/2] nbd: disable signals and forking on Windows builds Daniel P. Berrangé 2020-08-24 17:12 ` Eric Blake 2020-08-25 10:35 ` Daniel P. Berrangé 2020-08-25 5:35 ` Thomas Huth 2020-08-25 14:55 ` [PATCH 0/2] nbd: build qemu-nbd on Windows Daniel P. Berrangé
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).