Re: [Qemu-devel] [PATCH] Fix exit on 'pci_add' Monitor command

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> If the user issues one of the following commands to the Monitor:
>
> pci_add pci_addr=auto nic model=None
> pci_add pci_addr=auto nic model=?
>
> QEMU will exit, because the function used to perform sanity
> checks (qemu_check_nic_model_list()) exits on error.

Yes.  I meant to fix this, but you beat me to the line.

There might be more bugs like this one.

> This function is used by the startup code, where it makes
> sense to exit on error, but in the Monitor it doesn't.
>
> Changing qemu_check_nic_model_list() to not exit on error
> is not possible though, as it's used by the board init
> code (the PC one), where all board specific code must have
> void return.
>
> The way I've chosen to fix this was to introduce a new function
> called pci_nic_supported(), which checks if the NIC is supported
> and returns true or false accordingly.
>
> The new function is used only by the Monitor, it performs the
> necessary check and returns an error in case the NIC is not
> supported, thus qemu_check_nic_model_list()'s exit is never trigged.
>
> The following should be observed:
>
> 1. Only the specified NIC is checked, the default one is assumed
> to be supported
>
> 2. The NIC query command (model=?) won't work with pci_add, the
> right way to do this with the Monitor is to add a new command
>
> Signed-off-by: Luiz Capitulino <address@hidden>

It's a minimal fix, and I trust it works.  But is it the proper fix?

It works by checking the model before calling pci_nic_init() on behalf
of monitor cmmand "pci_add ... nic ...", so that when pci_nic_init()
checks the model again, it always succeeds, and thus never exits.

My minor complaint is that the new model check pci_nic_supported()
duplicates the existing check in qemu_check_nic_model_list().

My major complaint is that I'd rather see the code cleaned up there.
It's perfectly fine for code that can run only during startup to
terminate the program on configuration error.  Code to be used after
startup (used from monitor, in particular) must not do that.  Instead,
it should return failure up the call chain, until we reach either
startup code or monitor code, where the policy how to handle the error
resides.