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

[David Woodhouse]

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.

smime.p7s
Description: S/MIME cryptographic signature