| Message ID | 20200929125516.186715-4-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/export: add BlockExportOptions->iothread member | expand |
On 29/09/20 14:55, Stefan Hajnoczi wrote: > Block exports are used by softmmu, qemu-storage-daemon, and qemu-nbd. > They are not used by other programs and are not otherwise needed in > libblock. > > Undo the recent move of blockdev-nbd.c from blockdev_ss into block_ss. > Since bdrv_close_all() (libblock) calls blk_exp_close_all() > (libblockdev) a stub function is required.. > > Make qemu-ndb.c use signal handling utility functions instead of > duplicating the code. This helps because os-posix.c is in libblockdev > and it depends on a qemu_system_killed() symbol that qemu-nbd.c lacks. > Once we use the signal handling utility functions we also end up > providing the necessary symbol. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > qemu-nbd.c | 21 +++++++++------------ > stubs/blk-exp-close-all.c | 7 +++++++ > block/export/meson.build | 4 ++-- > meson.build | 4 ++-- > nbd/meson.build | 2 ++ > stubs/meson.build | 1 + > 6 files changed, 23 insertions(+), 16 deletions(-) > create mode 100644 stubs/blk-exp-close-all.c > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 6d7ac7490f..06774ca615 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -25,6 +25,7 @@ > #include "qapi/error.h" > #include "qemu/cutils.h" > #include "sysemu/block-backend.h" > +#include "sysemu/runstate.h" /* for qemu_system_killed() prototype */ > #include "block/block_int.h" > #include "block/nbd.h" > #include "qemu/main-loop.h" > @@ -155,7 +156,11 @@ QEMU_COPYRIGHT "\n" > } > > #if HAVE_NBD_DEVICE > -static void termsig_handler(int signum) > +/* > + * The client thread uses SIGTERM to interrupt the server. A signal > + * handler ensures that "qemu-nbd -v -c" exits with a nice status code. > + */ > +void qemu_system_killed(int signum, pid_t pid) > { > atomic_cmpxchg(&state, RUNNING, TERMINATE); > qemu_notify_event(); > @@ -581,20 +586,12 @@ int main(int argc, char **argv) > const char *pid_file_name = NULL; > BlockExportOptions *export_opts; > > + os_setup_early_signal_handling(); > + > #if HAVE_NBD_DEVICE > - /* The client thread uses SIGTERM to interrupt the server. A signal > - * handler ensures that "qemu-nbd -v -c" exits with a nice status code. > - */ > - struct sigaction sa_sigterm; > - memset(&sa_sigterm, 0, sizeof(sa_sigterm)); > - sa_sigterm.sa_handler = termsig_handler; > - sigaction(SIGTERM, &sa_sigterm, NULL); > + os_setup_signal_handling(); > #endif /* HAVE_NBD_DEVICE */ > > -#ifdef CONFIG_POSIX > - signal(SIGPIPE, SIG_IGN); > -#endif > - > socket_init(); > error_init(argv[0]); > module_call_init(MODULE_INIT_TRACE); > diff --git a/stubs/blk-exp-close-all.c b/stubs/blk-exp-close-all.c > new file mode 100644 > index 0000000000..1c71316763 > --- /dev/null > +++ b/stubs/blk-exp-close-all.c > @@ -0,0 +1,7 @@ > +#include "qemu/osdep.h" > +#include "block/export.h" > + > +/* Only used in programs that support block exports (libblockdev.fa) */ > +void blk_exp_close_all(void) > +{ > +} > diff --git a/block/export/meson.build b/block/export/meson.build > index 469a7aa0f5..a2772a0dce 100644 > --- a/block/export/meson.build > +++ b/block/export/meson.build > @@ -1,2 +1,2 @@ > -block_ss.add(files('export.c')) > -block_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-server.c')) > +blockdev_ss.add(files('export.c')) > +blockdev_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-server.c')) > diff --git a/meson.build b/meson.build > index 18d689b423..0e9528adab 100644 > --- a/meson.build > +++ b/meson.build > @@ -835,7 +835,6 @@ subdir('dump') > > block_ss.add(files( > 'block.c', > - 'blockdev-nbd.c', > 'blockjob.c', > 'job.c', > 'qemu-io-cmds.c', > @@ -848,6 +847,7 @@ subdir('block') > > blockdev_ss.add(files( > 'blockdev.c', > + 'blockdev-nbd.c', > 'iothread.c', > 'job-qmp.c', > )) > @@ -1171,7 +1171,7 @@ if have_tools > qemu_io = executable('qemu-io', files('qemu-io.c'), > dependencies: [block, qemuutil], install: true) > qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'), > - dependencies: [block, qemuutil], install: true) > + dependencies: [blockdev, qemuutil], install: true) > > subdir('storage-daemon') > subdir('contrib/rdmacm-mux') > diff --git a/nbd/meson.build b/nbd/meson.build > index 0c00a776d3..2baaa36948 100644 > --- a/nbd/meson.build > +++ b/nbd/meson.build > @@ -1,5 +1,7 @@ > block_ss.add(files( > 'client.c', > 'common.c', > +)) > +blockdev_ss.add(files( > 'server.c', > )) > diff --git a/stubs/meson.build b/stubs/meson.build > index e0b322bc28..0fdcf93c09 100644 > --- a/stubs/meson.build > +++ b/stubs/meson.build > @@ -1,6 +1,7 @@ > stub_ss.add(files('arch_type.c')) > stub_ss.add(files('bdrv-next-monitor-owned.c')) > stub_ss.add(files('blk-commit-all.c')) > +stub_ss.add(files('blk-exp-close-all.c')) > stub_ss.add(files('blockdev-close-all-bdrv-states.c')) > stub_ss.add(files('change-state-handler.c')) > stub_ss.add(files('clock-warp.c')) > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
On 9/29/20 7:55 AM, Stefan Hajnoczi wrote: > Block exports are used by softmmu, qemu-storage-daemon, and qemu-nbd. > They are not used by other programs and are not otherwise needed in > libblock. > > Undo the recent move of blockdev-nbd.c from blockdev_ss into block_ss. > Since bdrv_close_all() (libblock) calls blk_exp_close_all() > (libblockdev) a stub function is required.. > > Make qemu-ndb.c use signal handling utility functions instead of nbd > duplicating the code. This helps because os-posix.c is in libblockdev > and it depends on a qemu_system_killed() symbol that qemu-nbd.c lacks. > Once we use the signal handling utility functions we also end up > providing the necessary symbol. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 9/29/20 7:55 AM, Stefan Hajnoczi wrote: > Block exports are used by softmmu, qemu-storage-daemon, and qemu-nbd. > They are not used by other programs and are not otherwise needed in > libblock. > > Undo the recent move of blockdev-nbd.c from blockdev_ss into block_ss. > Since bdrv_close_all() (libblock) calls blk_exp_close_all() > (libblockdev) a stub function is required.. > > Make qemu-ndb.c use signal handling utility functions instead of > duplicating the code. This helps because os-posix.c is in libblockdev > and it depends on a qemu_system_killed() symbol that qemu-nbd.c lacks. > Once we use the signal handling utility functions we also end up > providing the necessary symbol. Hmm. I just stumbled on a long-standing bug in qemu-nbd - it installs a SIGTERM handler, but not a SIGINT or SIGHUP handler. This matters in the following sequence: qemu-nbd -f qcow2 -B bitmap image # Ctrl-C qemu-nbd -f qcow2 -B bitmap image because the first instance dies with SIGINT but there is no handler installed, qemu-nbd does not release the bitmap from being marked in-use, and the second instance then fails with: qemu-nbd: Bitmap 'b0' is inconsistent and cannot be used And to my surprise, while I was trying to find the root cause to fixing the bug I just found, I noticed that your patch happens to fix that... > +++ b/qemu-nbd.c > @@ -581,20 +586,12 @@ int main(int argc, char **argv) > const char *pid_file_name = NULL; > BlockExportOptions *export_opts; > > + os_setup_early_signal_handling(); > + > #if HAVE_NBD_DEVICE > - /* The client thread uses SIGTERM to interrupt the server. A signal > - * handler ensures that "qemu-nbd -v -c" exits with a nice status code. > - */ > - struct sigaction sa_sigterm; > - memset(&sa_sigterm, 0, sizeof(sa_sigterm)); > - sa_sigterm.sa_handler = termsig_handler; > - sigaction(SIGTERM, &sa_sigterm, NULL); > + os_setup_signal_handling(); ...by installing a SIGINT handler. Is HAVE_NBD_DEVICE really the best gate for this code, or is it really whether we are compiling for mingw? At any rate, you may want to add a link to https://bugzilla.redhat.com/show_bug.cgi?id=1883608 in the commit message, and/or separate the bug fix out into a separate commit. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Sep 29, 2020 at 12:36:10PM -0500, Eric Blake wrote: > On 9/29/20 7:55 AM, Stefan Hajnoczi wrote: > > Block exports are used by softmmu, qemu-storage-daemon, and qemu-nbd. > > They are not used by other programs and are not otherwise needed in > > libblock. > > > > Undo the recent move of blockdev-nbd.c from blockdev_ss into block_ss. > > Since bdrv_close_all() (libblock) calls blk_exp_close_all() > > (libblockdev) a stub function is required.. > > > > Make qemu-ndb.c use signal handling utility functions instead of > > duplicating the code. This helps because os-posix.c is in libblockdev > > and it depends on a qemu_system_killed() symbol that qemu-nbd.c lacks. > > Once we use the signal handling utility functions we also end up > > providing the necessary symbol. > > Hmm. I just stumbled on a long-standing bug in qemu-nbd - it installs a > SIGTERM handler, but not a SIGINT or SIGHUP handler. This matters in > the following sequence: > > qemu-nbd -f qcow2 -B bitmap image # Ctrl-C > qemu-nbd -f qcow2 -B bitmap image > > because the first instance dies with SIGINT but there is no handler > installed, qemu-nbd does not release the bitmap from being marked > in-use, and the second instance then fails with: > > qemu-nbd: Bitmap 'b0' is inconsistent and cannot be used > > And to my surprise, while I was trying to find the root cause to fixing > the bug I just found, I noticed that your patch happens to fix that... > > > +++ b/qemu-nbd.c > > > @@ -581,20 +586,12 @@ int main(int argc, char **argv) > > const char *pid_file_name = NULL; > > BlockExportOptions *export_opts; > > > > + os_setup_early_signal_handling(); > > + > > #if HAVE_NBD_DEVICE > > - /* The client thread uses SIGTERM to interrupt the server. A signal > > - * handler ensures that "qemu-nbd -v -c" exits with a nice status code. > > - */ > > - struct sigaction sa_sigterm; > > - memset(&sa_sigterm, 0, sizeof(sa_sigterm)); > > - sa_sigterm.sa_handler = termsig_handler; > > - sigaction(SIGTERM, &sa_sigterm, NULL); > > + os_setup_signal_handling(); > > ...by installing a SIGINT handler. > > Is HAVE_NBD_DEVICE really the best gate for this code, or is it really > whether we are compiling for mingw? At any rate, you may want to add a > link to https://bugzilla.redhat.com/show_bug.cgi?id=1883608 in the > commit message, and/or separate the bug fix out into a separate commit. Thanks for letting me know about the bug report. The link can be added to the commit description when merging (or if I resend). Stefan
diff --git a/qemu-nbd.c b/qemu-nbd.c index 6d7ac7490f..06774ca615 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -25,6 +25,7 @@ #include "qapi/error.h" #include "qemu/cutils.h" #include "sysemu/block-backend.h" +#include "sysemu/runstate.h" /* for qemu_system_killed() prototype */ #include "block/block_int.h" #include "block/nbd.h" #include "qemu/main-loop.h" @@ -155,7 +156,11 @@ QEMU_COPYRIGHT "\n" } #if HAVE_NBD_DEVICE -static void termsig_handler(int signum) +/* + * The client thread uses SIGTERM to interrupt the server. A signal + * handler ensures that "qemu-nbd -v -c" exits with a nice status code. + */ +void qemu_system_killed(int signum, pid_t pid) { atomic_cmpxchg(&state, RUNNING, TERMINATE); qemu_notify_event(); @@ -581,20 +586,12 @@ int main(int argc, char **argv) const char *pid_file_name = NULL; BlockExportOptions *export_opts; + os_setup_early_signal_handling(); + #if HAVE_NBD_DEVICE - /* The client thread uses SIGTERM to interrupt the server. A signal - * handler ensures that "qemu-nbd -v -c" exits with a nice status code. - */ - struct sigaction sa_sigterm; - memset(&sa_sigterm, 0, sizeof(sa_sigterm)); - sa_sigterm.sa_handler = termsig_handler; - sigaction(SIGTERM, &sa_sigterm, NULL); + os_setup_signal_handling(); #endif /* HAVE_NBD_DEVICE */ -#ifdef CONFIG_POSIX - signal(SIGPIPE, SIG_IGN); -#endif - socket_init(); error_init(argv[0]); module_call_init(MODULE_INIT_TRACE); diff --git a/stubs/blk-exp-close-all.c b/stubs/blk-exp-close-all.c new file mode 100644 index 0000000000..1c71316763 --- /dev/null +++ b/stubs/blk-exp-close-all.c @@ -0,0 +1,7 @@ +#include "qemu/osdep.h" +#include "block/export.h" + +/* Only used in programs that support block exports (libblockdev.fa) */ +void blk_exp_close_all(void) +{ +} diff --git a/block/export/meson.build b/block/export/meson.build index 469a7aa0f5..a2772a0dce 100644 --- a/block/export/meson.build +++ b/block/export/meson.build @@ -1,2 +1,2 @@ -block_ss.add(files('export.c')) -block_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-server.c')) +blockdev_ss.add(files('export.c')) +blockdev_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-server.c')) diff --git a/meson.build b/meson.build index 18d689b423..0e9528adab 100644 --- a/meson.build +++ b/meson.build @@ -835,7 +835,6 @@ subdir('dump') block_ss.add(files( 'block.c', - 'blockdev-nbd.c', 'blockjob.c', 'job.c', 'qemu-io-cmds.c', @@ -848,6 +847,7 @@ subdir('block') blockdev_ss.add(files( 'blockdev.c', + 'blockdev-nbd.c', 'iothread.c', 'job-qmp.c', )) @@ -1171,7 +1171,7 @@ if have_tools qemu_io = executable('qemu-io', files('qemu-io.c'), dependencies: [block, qemuutil], install: true) qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'), - dependencies: [block, qemuutil], install: true) + dependencies: [blockdev, qemuutil], install: true) subdir('storage-daemon') subdir('contrib/rdmacm-mux') diff --git a/nbd/meson.build b/nbd/meson.build index 0c00a776d3..2baaa36948 100644 --- a/nbd/meson.build +++ b/nbd/meson.build @@ -1,5 +1,7 @@ block_ss.add(files( 'client.c', 'common.c', +)) +blockdev_ss.add(files( 'server.c', )) diff --git a/stubs/meson.build b/stubs/meson.build index e0b322bc28..0fdcf93c09 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -1,6 +1,7 @@ stub_ss.add(files('arch_type.c')) stub_ss.add(files('bdrv-next-monitor-owned.c')) stub_ss.add(files('blk-commit-all.c')) +stub_ss.add(files('blk-exp-close-all.c')) stub_ss.add(files('blockdev-close-all-bdrv-states.c')) stub_ss.add(files('change-state-handler.c')) stub_ss.add(files('clock-warp.c'))
Block exports are used by softmmu, qemu-storage-daemon, and qemu-nbd. They are not used by other programs and are not otherwise needed in libblock. Undo the recent move of blockdev-nbd.c from blockdev_ss into block_ss. Since bdrv_close_all() (libblock) calls blk_exp_close_all() (libblockdev) a stub function is required.. Make qemu-ndb.c use signal handling utility functions instead of duplicating the code. This helps because os-posix.c is in libblockdev and it depends on a qemu_system_killed() symbol that qemu-nbd.c lacks. Once we use the signal handling utility functions we also end up providing the necessary symbol. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- qemu-nbd.c | 21 +++++++++------------ stubs/blk-exp-close-all.c | 7 +++++++ block/export/meson.build | 4 ++-- meson.build | 4 ++-- nbd/meson.build | 2 ++ stubs/meson.build | 1 + 6 files changed, 23 insertions(+), 16 deletions(-) create mode 100644 stubs/blk-exp-close-all.c