Re: [PATCH 2/2] nbd: disable signals and forking on Windows builds
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é > > --- > > 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(, 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 > > 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
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é --- 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(, 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(_sigterm, 0, sizeof(sa_sigterm)); sa_sigterm.sa_handler = termsig_handler; sigaction(SIGTERM, _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 2/2] nbd: disable signals and forking on Windows builds
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é --- 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(, 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 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
[PATCH 2/2] nbd: disable signals and forking on Windows builds
Disabling these parts are sufficient to get the qemu-nbd program compiling in a Windows build. Signed-off-by: Daniel P. Berrangé --- 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(, 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(_sigterm, 0, sizeof(sa_sigterm)); sa_sigterm.sa_handler = termsig_handler; sigaction(SIGTERM, _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