Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()

[Thomas Huth]

On 26/01/2024 12.25, David Woodhouse wrote:
> On Fri, 2024-01-26 at 12:20 +0100, Thomas Huth wrote:
> > On 26/01/2024 12.13, David Woodhouse wrote:
> > > On Fri, 2024-01-26 at 11:43 +0100, Thomas Huth wrote:
> > > > On 08/01/2024 21.26, David Woodhouse wrote:
> > > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > > >
> > > > > Eliminate direct access to nd_table[] and nb_nics by processing the the
> > > > > Xen and ISA NICs first and then calling pci_init_nic_devices() for the
> > > > > rest.
> > > > >
> > > > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > > > > Reviewed-by: Paul Durrant <paul@xen.org>
> > > > > ---
> > > > >     hw/i386/pc.c                | 26 ++++++++++++++++----------
> > > > >     include/hw/net/ne2000-isa.h |  2 --
> > > > >     2 files changed, 16 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > > index 496498df3a..d80c536d88 100644
> > > > > --- a/hw/i386/pc.c
> > > > > +++ b/hw/i386/pc.c
> > > > > @@ -658,8 +658,11 @@ static void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
> > > > >     {
> > > > >         static int nb_ne2k = 0;
> > > > >     
> > > > > -    if (nb_ne2k == NE2000_NB_MAX)
> > > > > +    if (nb_ne2k == NE2000_NB_MAX) {
> > > > > +        error_setg(&error_fatal,
> > > > > +                   "maximum number of ISA NE2000 devices exceeded");
> > > > >             return;
> > > > > +    }
> > > >
> > > > error_setg(&error_fatal, ...) quits QEMU, so the "return;" does not make
> > > > much sense anymore.
> > > > Now, according to include/qapi/error.h :
> > > >
> > > >     * Please don't error_setg(&error_fatal, ...), use error_report() and
> > > >     * exit(), because that's more obvious.
> > > >
> > > > So I'd suggest to do that instead.
> > >
> > > It's going slightly in the opposite direction to what's requested in
> > > https://lore.kernel.org/qemu-devel/34e2c0c6-4e04-486a-8e1f-4afdc461a5d4@linaro.org/
> > >
> > > I was thinking that a future patch would let the &error_fatal be an
> > > Error** passed in by the caller, and not actually hard-coded to be
> > > fatal at all.
> > >
> > > But sure, unless Philippe objects I'm happy to do it as you show above.
> >
> > Now that you mention it, I'd also prefer having an Error** parameter to the
> > function instead, that's certainly cleaner. So if you don't mind, please
> > follow Philippe's suggestion instead!
>
> Right. There's a whole bunch of functions to untangle, that take an
> Error** but don't return success/failure independently as they should.
> Or don't even take the Error**.
>
> Rather than trying to fix that as part of this series, this was my
> compromise — making it easy to switch that explicit &error_fatal out
> for a function parameter, but not trying to shave that part of the yak
> myself just yet.

I think the nicest compromise is to add the "Error **errp" to the 
pc_init_ne2k_isa() and change the caller to pass in &error_fatal there ... 
further clean-up (passing the error even up further in the stack) is out of 
scope of this series, indeed.

 Thomas