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

[Luiz Capitulino]

On Thu, 24 Sep 2009 20:12:30 +0200
Markus Armbruster <address@hidden> wrote:

> 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.

 Yeah.

> > 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?

 Well, given the current situation I agree it's not perfect but
I think it's good enough.

> 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().

 Yes, I don't like this either.. Although qemu_check_nic_model_list()
also checks for the default model, this is a bonus check. :)

> 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.

 What cleanup do you suggest?

 Note that it's not only about exit(), the function also has some
fprintf()s. If a big refactor is needed to properly fix this,
I guess we will have to live with the bug for a long time...