[Qemu-devel] PATCH: dont call exit() from pci_nic_init(), let caller handle

[Qemu-devel] PATCH: dont call exit() from pci_nic_init(), let caller handle

[Ryan Harper]

While exploring the PCI hotplug code recently posted, I encountered a
situation where I don't believe the current behavior is ideal.  With
hotplug, we can add additional pci-based nic devices like e1000 and
rtl8139 from the qemu monitor.  If one mistakenly specifies model=ne2000
(the ISA version), qemu just exits.  If a command is run from the
monitor and specifies bogus values, I don't believe the right behavior
is to exit out of the guest entirely.  The attached patch (which doesn't
apply directly against qemu-cvs since hotplug hasn't been merged)
changes pci_nic_init() to return NULL on error instead of exiting
and then I've replaced all callers to check the return value and exit(),
preserving the existing behavior, but allowing flexibility so
hotplug can do the right thing and just report the error rather than
exiting the guest.

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com


diffstat output:
 ipf.c         |    3 ++-
 mips_malta.c  |    6 ++++--
 pc.c          |    3 ++-
 pci.c         |    4 ++--
 ppc_chrp.c    |    3 ++-
 ppc_prep.c    |    3 ++-
 realview.c    |    3 ++-
 sun4u.c       |    3 ++-
 versatilepb.c |    3 ++-
 9 files changed, 20 insertions(+), 11 deletions(-)

---
Don't call exit() in the pci_nit_init() function, let the caller handle it.
This prevents silly things like device-hotplug trying to add a bogus nic and
exiting a running guest.  That shouldn't happen.  All callsites have been fix to
keep the existing behavior of exiting on a bogus nic, but device-hotplug.c can
handle bogus nic hot-adds without exiting the guest.

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>

diff --git a/qemu/hw/ipf.c b/qemu/hw/ipf.c
index 8c5304d..a84e343 100644
--- a/qemu/hw/ipf.c
+++ b/qemu/hw/ipf.c
@@ -560,7 +560,8 @@ static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
         } else if (pci_enabled) {
             if (strcmp(nd->model, "?") == 0)
                 fprintf(stderr, "qemu: Supported ISA NICs: ne2k_isa\n");
-            pci_nic_init(pci_bus, nd, -1);
+            if (!pci_nic_init(pci_bus, nd, -1))
+                exit(1);
         } else if (strcmp(nd->model, "?") == 0) {
             fprintf(stderr, "qemu: Supported ISA NICs: ne2k_isa\n");
             exit(1);
diff --git a/qemu/hw/mips_malta.c b/qemu/hw/mips_malta.c
index d39a02b..a50d2b9 100644
--- a/qemu/hw/mips_malta.c
+++ b/qemu/hw/mips_malta.c
@@ -495,9 +495,11 @@ static void network_init (PCIBus *pci_bus)
         }
         if (i == 0  && strcmp(nd->model, "pcnet") == 0) {
             /* The malta board has a PCNet card using PCI SLOT 11 */
-            pci_nic_init(pci_bus, nd, 88);
+            if (!pci_nic_init(pci_bus, nd, 88))
+                exit(1);
         } else {
-            pci_nic_init(pci_bus, nd, -1);
+            if (!pci_nic_init(pci_bus, nd, -1))
+                exit(1);
         }
     }
 }
diff --git a/qemu/hw/pc.c b/qemu/hw/pc.c
index 1122b87..0d2e6c3 100644
--- a/qemu/hw/pc.c
+++ b/qemu/hw/pc.c
@@ -1016,7 +1016,8 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
         } else if (pci_enabled) {
             if (strcmp(nd->model, "?") == 0)
                 fprintf(stderr, "qemu: Supported ISA NICs: ne2k_isa\n");
-            pci_nic_init(pci_bus, nd, -1);
+            if (!pci_nic_init(pci_bus, nd, -1))
+                exit(1);
         } else if (strcmp(nd->model, "?") == 0) {
             fprintf(stderr, "qemu: Supported ISA NICs: ne2k_isa\n");
             exit(1);
diff --git a/qemu/hw/pci.c b/qemu/hw/pci.c
index e587410..b8f4fbb 100644
--- a/qemu/hw/pci.c
+++ b/qemu/hw/pci.c
@@ -691,10 +691,10 @@ PCIDevice *pci_nic_init(PCIBus *bus, NICInfo *nd, int devfn)
     } else if (strcmp(nd->model, "?") == 0) {
         fprintf(stderr, "qemu: Supported PCI NICs: i82551 i82557b i82559er"
                         " ne2k_pci pcnet rtl8139 e1000 virtio\n");
-        exit (1);
+        return NULL;
     } else {
         fprintf(stderr, "qemu: Unsupported NIC: %s\n", nd->model);
-        exit (1);
+        return NULL;
     }
     nd->devfn = pci_dev->devfn;
     return pci_dev;
diff --git a/qemu/hw/ppc_chrp.c b/qemu/hw/ppc_chrp.c
index 4437a10..ff09c6a 100644
--- a/qemu/hw/ppc_chrp.c
+++ b/qemu/hw/ppc_chrp.c
@@ -268,7 +268,8 @@ static void ppc_core99_init (int ram_size, int vga_ram_size,
     for(i = 0; i < nb_nics; i++) {
         if (!nd_table[i].model)
             nd_table[i].model = "ne2k_pci";
-        pci_nic_init(pci_bus, &nd_table[i], -1);
+        if (!pci_nic_init(pci_bus, &nd_table[i], -1))
+            exit(1);
     }
     if (drive_get_max_bus(IF_IDE) >= MAX_IDE_BUS) {
         fprintf(stderr, "qemu: too many IDE bus\n");
diff --git a/qemu/hw/ppc_prep.c b/qemu/hw/ppc_prep.c
index c42688e..8b21ff2 100644
--- a/qemu/hw/ppc_prep.c
+++ b/qemu/hw/ppc_prep.c
@@ -676,7 +676,8 @@ static void ppc_prep_init (int ram_size, int vga_ram_size,
             || strcmp(nd_table[i].model, "ne2k_isa") == 0) {
             isa_ne2000_init(ne2000_io[i], i8259[ne2000_irq[i]], &nd_table[i]);
         } else {
-            pci_nic_init(pci_bus, &nd_table[i], -1);
+            if (!pci_nic_init(pci_bus, &nd_table[i], -1))
+                exit(1);
         }
     }
 
diff --git a/qemu/hw/realview.c b/qemu/hw/realview.c
index 29579d8..edae2f0 100644
--- a/qemu/hw/realview.c
+++ b/qemu/hw/realview.c
@@ -121,7 +121,8 @@ static void realview_init(int ram_size, int vga_ram_size,
         if (strcmp(nd->model, "smc91c111") == 0) {
             smc91c111_init(nd, 0x4e000000, pic[28]);
         } else {
-            pci_nic_init(pci_bus, nd, -1);
+            if (!pci_nic_init(pci_bus, nd, -1))
+                    exit(1);
         }
     }
 
diff --git a/qemu/hw/sun4u.c b/qemu/hw/sun4u.c
index 183f64a..e1a5985 100644
--- a/qemu/hw/sun4u.c
+++ b/qemu/hw/sun4u.c
@@ -342,7 +342,8 @@ static void sun4u_init(int ram_size, int vga_ram_size,
     for(i = 0; i < nb_nics; i++) {
         if (!nd_table[i].model)
             nd_table[i].model = "ne2k_pci";
-        pci_nic_init(pci_bus, &nd_table[i], -1);
+        if (!pci_nic_init(pci_bus, &nd_table[i], -1))
+            exit(1);
     }
 
     irq = qemu_allocate_irqs(dummy_cpu_set_irq, NULL, 32);
diff --git a/qemu/hw/versatilepb.c b/qemu/hw/versatilepb.c
index da6e4ec..a918d9e 100644
--- a/qemu/hw/versatilepb.c
+++ b/qemu/hw/versatilepb.c
@@ -201,7 +201,8 @@ static void versatile_init(int ram_size, int vga_ram_size,
         if (strcmp(nd->model, "smc91c111") == 0) {
             smc91c111_init(nd, 0x10010000, sic[25]);
         } else {
-            pci_nic_init(pci_bus, nd, -1);
+            if (!pci_nic_init(pci_bus, nd, -1))
+                exit(1);
         }
     }
     if (usb_enabled) {

[Qemu-devel] Re: [kvm-devel] PATCH: dont call exit() from pci_nic_init(), let caller handle

[Marcelo Tosatti]

On Wed, Mar 19, 2008 at 07:19:51PM -0500, Ryan Harper wrote:
> While exploring the PCI hotplug code recently posted, I encountered a
> situation where I don't believe the current behavior is ideal.  With
> hotplug, we can add additional pci-based nic devices like e1000 and
> rtl8139 from the qemu monitor.  If one mistakenly specifies model=ne2000
> (the ISA version), qemu just exits.  If a command is run from the
> monitor and specifies bogus values, I don't believe the right behavior
> is to exit out of the guest entirely.  The attached patch (which doesn't
> apply directly against qemu-cvs since hotplug hasn't been merged)
> changes pci_nic_init() to return NULL on error instead of exiting
> and then I've replaced all callers to check the return value and exit(),
> preserving the existing behavior, but allowing flexibility so
> hotplug can do the right thing and just report the error rather than
> exiting the guest.

Hi Ryan,

Looks good, thanks.

There might still be some exit()'s lurking around due to device/cpu hot/add
failure.

Re: [Qemu-devel] PATCH: dont call exit() from pci_nic_init(), let caller handle

[Avi Kivity]

Ryan Harper wrote:
> While exploring the PCI hotplug code recently posted, I encountered a
> situation where I don't believe the current behavior is ideal.  With
> hotplug, we can add additional pci-based nic devices like e1000 and
> rtl8139 from the qemu monitor.  If one mistakenly specifies model=ne2000
> (the ISA version), qemu just exits.  If a command is run from the
> monitor and specifies bogus values, I don't believe the right behavior
> is to exit out of the guest entirely.  The attached patch (which doesn't
> apply directly against qemu-cvs since hotplug hasn't been merged)
> changes pci_nic_init() to return NULL on error instead of exiting
> and then I've replaced all callers to check the return value and exit(),
> preserving the existing behavior, but allowing flexibility so
> hotplug can do the right thing and just report the error rather than
> exiting the guest.
>
>   

Applied, thanks.

[this didn't make it to kvm-devel for some reason?]

-- 
error compiling committee.c: too many arguments to function

Re: [kvm-devel] [Qemu-devel] PATCH: dont call exit() from pci_nic_init(), let caller handle

[Ryan Harper]

* Avi Kivity <avi@qumranet.com> [2008-03-20 07:19]:
> Ryan Harper wrote:
> > While exploring the PCI hotplug code recently posted, I encountered a
> > situation where I don't believe the current behavior is ideal.  With
> > hotplug, we can add additional pci-based nic devices like e1000 and
> > rtl8139 from the qemu monitor.  If one mistakenly specifies model=ne2000
> > (the ISA version), qemu just exits.  If a command is run from the
> > monitor and specifies bogus values, I don't believe the right behavior
> > is to exit out of the guest entirely.  The attached patch (which doesn't
> > apply directly against qemu-cvs since hotplug hasn't been merged)
> > changes pci_nic_init() to return NULL on error instead of exiting
> > and then I've replaced all callers to check the return value and exit(),
> > preserving the existing behavior, but allowing flexibility so
> > hotplug can do the right thing and just report the error rather than
> > exiting the guest.
> >
> >   
> 
> Applied, thanks.
> 
> [this didn't make it to kvm-devel for some reason?]

Yeah, not sure about that, sometimes it gets clogged in our outgoing
system; they tend to not get along with some servers for unknown reasons
to me.  It has worked in the past for me.  *shrugs*

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com