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

[Markus Armbruster]

Amit Shah <address@hidden> writes:

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

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.