Re: [Qemu-devel] [Qemu-block] Clean Block Driver Shutdown

[Markus Armbruster]

Peter Lieven <address@hidden> writes:

> Am 07.11.2017 um 11:22 schrieb Markus Armbruster:
>> Stefan Hajnoczi <address@hidden> writes:
>>
>>> On Tue, Oct 17, 2017 at 01:46:25PM +0200, Kevin Wolf wrote:
>>>> Am 17.10.2017 um 12:33 hat Peter Lieven geschrieben:
>>>>> I noticed that Qemu quits at several points with an exit() if the
>>>>> supplied parameters in the commandline are incorrect. This at some
>>>>> stages happens after there have already been connections to storage
>>>>> backends established.
>>>> Maybe we need to come to the conclusion that exit() is always wrong,
>>>> even during the initialisation.
>>>>
>>>>> These connections are not cleanly shut down in this case. For posix
>>>>> file backends that doesn't matter, but for other backends this leads
>>>>> to errors. E.g. iSCSI Targets log an aborted iSCSI connection due to
>>>>> tcp reset.
>>>>>
>>>>> I wonder what is the best way to fix this. A simply call to
>>>>> bdrv_close_all() in an atexit handler seems to work.  But is this a
>>>>> good solution? Maybe register this handler only until the VM starts.
>>>>> Or do we need an atexit handler in each block driver that requires a
>>>>> clean shutdown?
>>>> No, definitely not code in every single block driver. We need to make
>>>> sure to properly clean up what has been started.
>>>>
>>>> An atexit handler is probably relatively easy. I think it would be
>>>> cleaner to have proper error paths even in main(), like in every other
>>>> function. I'm not sure if this would be reasonably easy to achieve,
>>>> though.
>>> I agree that converting from exit(3) to real error handling is cleanest.
>>> Doing so would also be a good opportunity to consolidate ad-hoc
>>> fprintf(stderr) and error_report() calls.
>> error_report() & exit() assume a certain context.  They're good enough
>> when the assumption obviously holds.
>>
>> We also use them in code that can run when the assumption doesn't hold.
>> Sometimes because the code acquired new users, sometimes because the
>> code was always wrong.  Regardless, these are bugs in need of fixing.
>>
>> We also use them in code that currently happens to run only when the
>> assumption holds.  Trap for the unwary, cleanup can make sense, but is
>> hardly a priority.
>
>
> Of course, no priority, but how would you then handle block drivers
> that need a clean shutdown? Register atexit handlers for them?

Cleanup for cleanup's sake is not a priority.  But that's not why you're
considering it: you could use it to fix a bug (failure to finalize
certain resources on certain errors), so you don't have to atexit().
Judgement call.