Re: [libvirt PATCH v8 10/37] qemu: add functions to start and stop nbdkit

On Thu, Sep 21, 2023 at 12:50:52 -0500, Jonathon Jongsma wrote: > On 9/20/23 7:24 AM, Pavel Hrdina wrote: >> On Thu, Aug 31, 2023 at 04:39:50PM -0500, Jonathon Jongsma wrote: >>> Add some helper functions to build a virCommand object and run the >>> nbdkit process for a given virStorageSource. >>> >>> Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; >>> Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; >>> --- >>> src/qemu/qemu_nbdkit.c | 250 +++++++++++++++++++++++++++++++++++++++++ >>> src/qemu/qemu_nbdkit.h | 10 ++ >>> 2 files changed, 260 insertions(+) [...] >>> + VIR_DEBUG("Stopping nbdkit process %i", proc->pid); >>> + virProcessKill(proc->pid, SIGTERM); >> >> Coverity complains here that the return value of virProcessKill() is not >> checked which leads me to a question if we should use >> virProcessKillPainfully() instead. >> >> With the code that is pushed the function qemuNbdkitProcessStop() is >> called only within the qemu_nbdkit.c for these cases: >> >> - in qemuNbdkitProcessRestart() before starting the process again >> where we do not check if the original process was killed correctly >> or not, >> >> - in qemuNbdkitStopStorageSource() where we check return value of >> qemuNbdkitProcessStop() but it will always be 0, >> >> - in qemuNbdkitProcessStart() as error path where we don't check any >> return value. >> >> To me it seems that the return value qemuNbdkitProcessStop can be changed >> to void as we always return 0 and use virProcessKillPainfully() or >> properly pass return value of virProcessKill() and check it for every >> use of qemuNbdkitProcessStop(). >> >> Pavel > > Good question. In one of my earlier series I had actually used > virProcessKillPainfully(), but changed it based on a suggestion from Peter > that it would be bad to kill it painfully if nbdkit was ever used in > read-write mode. But apparently I forgot to handle a shutdown failure. An > alternative would be to simply refuse to use nbdkit if the user requests > read-write mode. We can use the same algorithm as with the qemu process where we first issue SIGTERM, thus if the process is responsive it can execute the shutdown actions. Otherwise it'll get SIGKILL right after.