qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Thomas Huth
Subject: Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
Date: Fri, 26 Jan 2024 14:45:44 +0100
User-agent: Mozilla Thunderbird

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]