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

[Jonathon Jongsma]

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

That sounds almost identical to what virProcessKillPainfully() does.

Jonathon