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

 Thanks,
  Thomas