On 9/21/23 1:10 PM, Peter Krempa wrote:
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@xxxxxxxxxx>
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
---
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.
That sounds almost identical to what virProcessKillPainfully() does.
Jonathon
