Re: [PATCH v3 01/46] net: add qemu_{configure, create}_nic_device(), qemu_find_nic_info()

[Thomas Huth <thuth@xxxxxxxxxx>]

On 26/01/2024 15.34, David Woodhouse wrote:
> On Fri, 2024-01-26 at 15:24 +0100, Thomas Huth wrote:
> > On 26/01/2024 15.16, David Woodhouse wrote:
> > > On Fri, 2024-01-26 at 12:10 +0100, Thomas Huth wrote:
> > > > > +/* "Please create a device, if you have a configuration for it" */
> > > > > +DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
> > > > > +                                    const char *alias)
> > > > > +{
> > > > > +    NICInfo *nd = qemu_find_nic_info(typename, match_default, alias);
> > > > > +    DeviceState *dev;
> > > > > +
> > > > > +    if (!nd) {
> > > > > +        return NULL;
> > > > > +    }
> > > > The qemu_check_nic_model() function that was used in some code that you
> > > > turned into qemu_create_nic_device() used to set:
> > > >
> > > >        if (!nd->model)
> > > >            nd->model = g_strdup(default_model);
> > > >
> > > > (in the qemu_find_nic_model() function that has been called by
> > > > qemu_check_nic_model())
> > > >
> > > > Should we do that also here to make sure that nd->model is not NULL afterwards?
> > > Good question, but I don't think we care. The qdev_set_nic_properties()
> > > function certainly doesn't propagate nd->model to anywhere.
> > >
> > > I renamed nd->model to nd->modelname in a patch shown below, just to be
> > > 100% sure I'm not missing any other code paths which might consume it.
> > Ok, thanks for checking! Maybe mention it in the patch description in v4, so
> > that we've got it recorded somewhere that nd->model might be left at NULL
> > afterwards, but that there are no further consumers, so it should be fine.
> Makes sense.
>
> https://git.infradead.org/?p=users/dwmw2/qemu.git;a=commitdiff;h=694f82bc09080
> now says:
>
>
> net: add qemu_{configure,create}_nic_device(), qemu_find_nic_info()
>
> Most code which directly accesses nd_table[] and nb_nics uses them for
> one of two things. Either "I have created a NIC device and I'd like a
> configuration for it", or "I will create a NIC device *if* there is a
> configuration for it".  With some variants on the theme around whether
> they actually *check* if the model specified in the configuration is
> the right one.
>
> Provide functions which perform both of those, allowing platforms to
> be a little more consistent and as a step towards making nd_table[]
> and nb_nics private to the net code.
>
> One might argue that platforms ought to be consistent about whether
> they create the unconfigured devices or not, but making significant
> user-visible changes is explicitly *not* the intent right now.
>
> The new functions leave the 'model' field of the NICInfo as NULL after
> using it for the default NIC model, unlike the qemu_check_nic_model()
> function which does set nd->model to match default_model explicitly.
> This is acceptable because there is no code which consumes nd->model
> except this NIC-matching code in net/net.c, and no reasonable excuse
> for any code wanting to use nd->model in future.
With that feel free to add:
Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx>