Re: [PATCH v3 2/4] python/machine: raise VMLaunchFailure exception from launch()

[Hanna Reitz]

On 24.01.22 19:06, John Snow wrote:
> This allows us to pack in some extra information about the failure,
> which guarantees that if the caller did not *intentionally* cause a
> failure (by capturing this Exception), some pretty good clues will be
> printed at the bottom of the traceback information.

OK, I presume in contrast to unconditionally logging this on debug 
level, which is less than ideal because on that level it’s most likely 
hidden, but that was exactly the point, because we don’t know whether 
the caller will catch the exception, so we mustn’t log it on a more 
urgent level.

But by creating a new exception class, we get a reasonable log output 
exactly when the caller won’t catch it.

> This will help make failures in the event of a non-negative return code
> more obvious when they go unhandled; the current behavior is to print a
> warning message only in the event of signal-based terminations (for
> negative return codes).

I assume you mean the one in _post_shutdown()...?

Confused me a bit, because for a while I interpreted this to mean “We 
don’t output anything in case of a positive return code”, but it means 
“We don’t print any details in that case, because the exception we 
re-raise in launch() doesn’t contain valuable information”.  Makes sense.

> (Note: In Python, catching BaseException instead of Exception is like
> installing a signal handler that will run as long as Python itself
> doesn't crash.

This really confused me, because I can’t really understand this at all.

But I guess what I took from googling was that every exception object 
must be derived from BaseException eventually, and so we continue to 
catch all exceptions here, we just give them a name. (And then we create 
a VMLaunchFailure only for Exception exceptions, because the others 
don’t have much to do with launching the VM.)

> KeyboardInterrupt and several other "strong" events in
> Python are a BaseException. These events should generally not be
> suppressed, but occasionally we want to perform some cleanup in response
> to one.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/machine/machine.py            | 45 ++++++++++++++++++++---
>   tests/qemu-iotests/tests/mirror-top-perms |  3 +-
>   2 files changed, 40 insertions(+), 8 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

(Looked at `except` and `ConnectError` usage outside of 
mirror-top-perms, but couldn’t find anything else that looked like it 
caught VM launch exceptions.)