Amit Shah <amit.s...@redhat.com> writes: > On (Wed) Jun 09 2010 [08:37:26], Markus Armbruster wrote: >> Amit Shah <amit.s...@redhat.com> writes: >> >> > On (Tue) Jun 08 2010 [18:33:00], Markus Armbruster wrote: >> >> Amit Shah <amit.s...@redhat.com> 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.
Ah! Could a brief version of this be added to the commit message? >> >> > 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. Okay. Thanks for your patience.