Re: [libvirt PATCH v8 25/37] qemu: Monitor nbdkit process for exit

On Thu, Aug 31, 2023 at 16:40:05 -0500, Jonathon Jongsma wrote: > Adds the ability to monitor the nbdkit process so that we can take > action in case the child exits unexpectedly. > > When the nbdkit process exits, we pause the vm, restart nbdkit, and then > resume the vm. This allows the vm to continue working in the event of a > nbdkit failure. > > Eventually we may want to generalize this functionality since we may > need something similar for e.g. qemu-storage-daemon, etc. > > The process is monitored with the pidfd_open() syscall if it exists > (since linux 5.3). Otherwise it resorts to checking whether the process > is alive once a second. The one-second time period was chosen somewhat > arbitrarily. > > Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; > --- > meson.build | 11 +++ > src/qemu/qemu_domain.c | 1 + > src/qemu/qemu_domain.h | 1 + > src/qemu/qemu_driver.c | 16 ++++ > src/qemu/qemu_nbdkit.c | 169 ++++++++++++++++++++++++++++++++++++--- > src/qemu/qemu_nbdkit.h | 8 +- > src/qemu/qemu_process.c | 15 +++- > src/qemu/qemu_process.h | 3 + > tests/meson.build | 6 +- > tests/qemuxml2argvtest.c | 6 +- > 10 files changed, 219 insertions(+), 17 deletions(-) [...] > diff --git a/tests/meson.build b/tests/meson.build > index f05774263c..b235c5f4dd 100644 > --- a/tests/meson.build > +++ b/tests/meson.build > @@ -456,8 +456,12 @@ if conf.has('WITH_QEMU') > { 'name': 'qemuvhostusertest', 'link_with': [ test_qemu_driver_lib ], 'link_whole': [ test_file_wrapper_lib ] }, > { 'name': 'qemuxml2argvtest', 'link_with': [ test_qemu_driver_lib, test_utils_qemu_monitor_lib ], 'link_whole': [ test_utils_qemu_lib, test_file_wrapper_lib ] }, > { 'name': 'qemuxml2xmltest', 'link_with': [ test_qemu_driver_lib ], 'link_whole': [ test_utils_qemu_lib, test_file_wrapper_lib ] }, > - { 'name': 'qemunbdkittest', 'link_with': [ test_qemu_driver_lib ], 'link_whole': [ test_utils_qemu_lib, test_file_wrapper_lib ] }, > ] > + if conf.has('WITH_NBDKIT') > + tests += [ > + { 'name': 'qemunbdkittest', 'link_with': [ test_qemu_driver_lib ], 'link_whole': [ test_utils_qemu_lib, test_file_wrapper_lib ] }, > + ] > + endif > endif > > if conf.has('WITH_REMOTE') > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index 0304f66f1d..216fd3a841 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -839,8 +839,12 @@ mymain(void) > # define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ > DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ARG_END) > > -# define DO_TEST_CAPS_LATEST_NBDKIT(name, ...) \ > +# if WITH_NBDKIT > +# define DO_TEST_CAPS_LATEST_NBDKIT(name, ...) \ > DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", ARG_NBDKIT_CAPS, __VA_ARGS__, QEMU_NBDKIT_CAPS_LAST, ARG_END) > +# else > +# define DO_TEST_CAPS_LATEST_NBDKIT(name, ...) > +# endif /* WITH_NBDKIT */ > > # define DO_TEST_CAPS_LATEST(name) \ > DO_TEST_CAPS_ARCH_LATEST(name, "x86_64") These two hunks seem misplaced and don't semantically align with the rest of the patch. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;