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

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

On 9/5/23 2:54 AM, Peter Krempa wrote:
> 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@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>


Yes, they don't appear very related on first glance. But this is the 
first commit that added a dependency on the pidfd_open syscall (and thus 
added the "WITH_NBDKIT" preprocessor symbol). Before this patch, nbdkit 
tests would run fine and generate an nbdkit commandline on any platform 
as long as the nbdkit capabilities were set. But after this patch, any 
platform that did not provide this syscall would fall back to using the 
qemu block driver and generate a different commandline, causing tests to 
fail.

Jonathon