Re: [Qemu-devel] [PATCH v2] net: Fix hotplug with pci_add

[Amit Shah]

On (Wed) Jun 09 2010 [08:37:26], Markus Armbruster wrote:
> Amit Shah <address@hidden> writes:
> 
> > On (Tue) Jun 08 2010 [18:33:00], Markus Armbruster wrote:
> >> Amit Shah <address@hidden> writes:
> >> 
> >> > The correct model type wasn't getting added when hotplugging nics with
> >> > pci_add.
> >> >
> >> > Testcase: start VM with default nic type. In the qemu_monitor:
> >> >
> >> > (qemu) pci_add auto nic model=virtio
> >> >
> >> > This results in a nic hot-plug of the same nic type as the default.
> >> 
> >> Works fine for me on master, fd1dc858370d9a9ac7ea2512812c3a152ee6484b.
> >> What am I doing wrong?
> >
> > Did you start with a virtio nic added? The 'default' here is the nic
> > type that's added as the first nic. Try this: start a VM with model
> > e1000 and use pci_add to add a nic type of virtio.
> 
> I do.  Nevertheless, "pci_add auto nic model=e1000" adds an e1000.
> 
> Also works if I start without a NIC.
> 
> Ah, if I start with a -net nic, then pci_add breaks as described!
> 
> Now my next question is *how* your patch fixes this.  It's not at all
> obvious to me.

The callers of net_client_init() expect a positive return value that
means an index into the nd_table[] array, where the options just parsed
are placed.

Let's look at 5294e2c774f120e10b44652ac143abda356f44eb, which broke this
thing:

 
             if (net_client_types[i].init) {
-                return net_client_types[i].init(opts, mon, name, vlan);
-            } else {
-                return 0;
+                if (net_client_types[i].init(opts, mon, name, vlan) < 0) {
+                    /* TODO push error reporting into init() methods */
+                    qerror_report(QERR_DEVICE_INIT_FAILED, type);
+                    return -1;
+                }
             }
+            return 0;

See it yet? instead of returning the value returned by
net_client_types[i].init(), we're now just returning either 0 or -1. The
return value is now by default '0' for successful init(), which causes
the calling functions to just end up using the same parameters that were
passed for the first nic type.

> >> > This was broken in 5294e2c774f120e10b44652ac143abda356f44eb
> >> >
> >> > Also changes the behaviour where no .init is defined for a
> >> > net_client_type. Previously, 0 was returned, which indicated the init
> >> > was successful and that 0 was the index into the nd_tables[] array.
> >> > Return -1, indicating unsuccessful init, in such a case.
> >> 
> >> The only element of net_client_types[] without an init() method is type
> >> "none", index 0.  So, doesn't this break -net none?  And what does it
> >> fix?
> >
> > The net_client_types[] index isn't relevant here. -net none works fine,
> > no problem.
> 
> Let me rephrase: Behavior changes for -net types without an init()
> method.  The only one without an init() method is "none".  Before,
> net_client_init() succeeded for it.  Now it fails.  What's the impact of
> that change?  And why does it make sense?

It makes sense because we don't actually initialise anything. We don't
place anything in the nd_table[] array. That means callers shouldn't
poke in the array for any values. Returning -1 makes sense for that
reason. If we continued to return 0, callers might just assume that init
was successful and that nd_table[0] was set up for use appropriately.

The thing is that the code doesn't go this far in case of '-net none'
anyway. This was just a potential bug lurking around for any new -net
method which didn't have an init.

                Amit