[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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>





 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.