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@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- 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.