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

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

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.

Also export the qemu_find_nic_info() helper, as some platforms have
special cases they need to handle.

Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
Reviewed-by: Paul Durrant <paul@xxxxxxx>

Attachment: smime.p7s
Description: S/MIME cryptographic signature