From: David Woodhouse <dwmw@amazon.co.uk>

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.

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

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
---
 include/net/net.h |  7 ++++++-
 net/net.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/include/net/net.h b/include/net/net.h
index ffbd2c8d56..25ea83fd12 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -207,7 +207,12 @@ int qemu_show_nic_models(const char *arg, const char *const *models);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
 int qemu_find_nic_model(NICInfo *nd, const char * const *models,
                         const char *default_model);
-
+NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
+                            const char *alias);
+bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
+                               const char *alias);
+DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
+                                    const char *alias);
 void print_net_client(Monitor *mon, NetClientState *nc);
 void net_socket_rs_init(SocketReadState *rs,
                         SocketReadStateFinalize *finalize,
diff --git a/net/net.c b/net/net.c
index 0520bc1681..aeb7f573fc 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1087,6 +1087,57 @@ static int net_init_nic(const Netdev *netdev, const char *name,
     return idx;
 }
 
+NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
+                            const char *alias)
+{
+    NICInfo *nd;
+    int i;
+
+    for (i = 0; i < nb_nics; i++) {
+        nd = &nd_table[i];
+
+        if (!nd->used || nd->instantiated) {
+            continue;
+        }
+
+        if ((match_default && !nd->model) || !g_strcmp0(nd->model, typename)
+            || (alias && !g_strcmp0(nd->model, alias))) {
+            return nd;
+        }
+    }
+    return NULL;
+}
+
+
+/* "I have created a device. Please configure it if you can" */
+bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
+                               const char *alias)
+{
+    NICInfo *nd = qemu_find_nic_info(object_get_typename(OBJECT(dev)),
+                                     match_default, alias);
+
+    if (nd) {
+        qdev_set_nic_properties(dev, nd);
+        return true;
+    }
+    return false;
+}
+
+/* "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;
+    }
+
+    dev = qdev_new(typename);
+    qdev_set_nic_properties(dev, nd);
+    return dev;
+}
 
 static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
     const Netdev *netdev,
-- 
2.41.0

On Mon, 8 Jan 2024 at 20:49, David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> 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.
>
> Also export the qemu_find_nic_info() helper, as some platforms have
> special cases they need to handle.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Paul Durrant <paul@xen.org>
> ---
>  include/net/net.h |  7 ++++++-
>  net/net.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/net.h b/include/net/net.h
> index ffbd2c8d56..25ea83fd12 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -207,7 +207,12 @@ int qemu_show_nic_models(const char *arg, const char *const *models);
>  void qemu_check_nic_model(NICInfo *nd, const char *model);
>  int qemu_find_nic_model(NICInfo *nd, const char * const *models,
>                          const char *default_model);
> -
> +NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
> +                            const char *alias);
> +bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
> +                               const char *alias);
> +DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
> +                                    const char *alias);

Could we have doc comments that document the purpose and API
for these new global functions, please?

thanks
-- PMM

On Fri, 2024-01-26 at 14:43 +0000, Peter Maydell wrote:
> 
> > +NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
> > +                            const char *alias);
> > +bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
> > +                               const char *alias);
> > +DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
> > +                                    const char *alias);
> 
> Could we have doc comments that document the purpose and API
> for these new global functions, please?

Like this? I deliberately fatfingered the argument names and didn't
even get a build warning, and I don't see any actual *documentation*
being generated with it...?

diff --git a/include/net/net.h b/include/net/net.h
index 25ea83fd12..14614b0a31 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -207,10 +207,46 @@ int qemu_show_nic_models(const char *arg, const char *const *models);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
 int qemu_find_nic_model(NICInfo *nd, const char * const *models,
                         const char *default_model);
+/**
+ * qemu_find_nic_info: Obtain NIC configuration information
+ * @typename: Name of device object type
+ * @match_default: Match NIC configurations with no model specified
+ * @alias: Additional model string to match (for user convenience and
+ *         backward compatibility).
+ *
+ * Search for a NIC configuration matching the NIC model constraints.
+ */
 NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
                             const char *alias);
+/**
+ * qemu_configure_nic_device: Apply NIC configuration to a given device
+ * @dev: Network device to be configured
+ * @match_default: Match NIC configurations with no model specified
+ * @alias: Additional model string to match
+ *
+ * Search for a NIC configuration for the provided device, using the
+ * additionally specified matching constraints. If found, apply the
+ * configuration using qdev_set_nic_properties() and return %true.
+ *
+ * This is used by platform code which creates the device anyway,
+ * regardless of whether there is a configuration for it. This tends
+ * to be platforms which ignore `--nodefaults` and create net devices
+ * anyway. This behaviour is not advised for new platforms; use the
+ * qemu_create_nic_device() function instead, which creates the device
+ * only when it is configured.
+ */
 bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
                                const char *alias);
+
+/**
+ * qemu_create_nic_device: Create a NIC device if a configuration exists for it
+ * @typename: Object typename of network device
+ * @match_default: Match NIC configurations with no model specified
+ * @alias: Additional model string to match
+ *
+ * Search for a NIC configuration for the provided device type. If found,
+ * create an object of the corresponding type and return it.
+ */
 DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
                                     const char *alias);
 void print_net_client(Monitor *mon, NetClientState *nc);

On Fri, 26 Jan 2024 at 15:20, David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Fri, 2024-01-26 at 14:43 +0000, Peter Maydell wrote:
> >
> > > +NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
> > > +                            const char *alias);
> > > +bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
> > > +                               const char *alias);
> > > +DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
> > > +                                    const char *alias);
> >
> > Could we have doc comments that document the purpose and API
> > for these new global functions, please?
>
> Like this? I deliberately fatfingered the argument names and didn't
> even get a build warning, and I don't see any actual *documentation*
> being generated with it...?

We use the doc comment format to allow for potential future
documentation generation, but it's only actually generated
if there's a .rst file somewhere under docs/ that has a
kernel-doc:: directive referencing the .h file (for instance
there's one in docs/devel/memory.rst that results in
https://www.qemu.org/docs/master/devel/memory.html#api-reference )

For almost all internal functions, we set the relatively low
bar of "have a doc comment so people reading the header file
can see what the functions do". Where there's a more complex
subsystem that merits its own hand-written documentation
under docs/devel, then if the author of that documentation
is enthusiastic they can clean up and pull in specific headers
to add autogenerated docs. But the primary audience is the
human reader of the .h file.

> diff --git a/include/net/net.h b/include/net/net.h
> index 25ea83fd12..14614b0a31 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -207,10 +207,46 @@ int qemu_show_nic_models(const char *arg, const char *const *models);
>  void qemu_check_nic_model(NICInfo *nd, const char *model);
>  int qemu_find_nic_model(NICInfo *nd, const char * const *models,
>                          const char *default_model);
> +/**
> + * qemu_find_nic_info: Obtain NIC configuration information
> + * @typename: Name of device object type
> + * @match_default: Match NIC configurations with no model specified
> + * @alias: Additional model string to match (for user convenience and
> + *         backward compatibility).
> + *
> + * Search for a NIC configuration matching the NIC model constraints.
> + */
>  NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
>                              const char *alias);
> +/**
> + * qemu_configure_nic_device: Apply NIC configuration to a given device
> + * @dev: Network device to be configured
> + * @match_default: Match NIC configurations with no model specified
> + * @alias: Additional model string to match
> + *
> + * Search for a NIC configuration for the provided device, using the
> + * additionally specified matching constraints. If found, apply the
> + * configuration using qdev_set_nic_properties() and return %true.
> + *
> + * This is used by platform code which creates the device anyway,
> + * regardless of whether there is a configuration for it. This tends
> + * to be platforms which ignore `--nodefaults` and create net devices
> + * anyway. This behaviour is not advised for new platforms; use the
> + * qemu_create_nic_device() function instead, which creates the device
> + * only when it is configured.

I disagree about this paragraph. The behaviour we want for new
platforms is:

 * If this is modelling some board where the ethernet device is
   always present (eg it is soldered on to the board, or it is
   a part of the SoC that the board uses), then always create
   that device
 * If the hardware being modelled has the ethernet device as an
   optional device (eg physically removable like a PCI card),
   then the board should arrange that --nodefaults causes it to
   not be created

Basically if the guest OS is entitled to assume the ethernet
device is present then we shouldn't allow the machine to be
created with it not present, because all that will happen is
that the guest will fall over in bootup.

(Similar applies to things like whether the board should
honour the option to disable USB support or not.)

thanks
-- PMM

On Fri, 2024-01-26 at 15:33 +0000, Peter Maydell wrote:
> On Fri, 26 Jan 2024 at 15:20, David Woodhouse <dwmw2@infradead.org> wrote:
> > 
> > On Fri, 2024-01-26 at 14:43 +0000, Peter Maydell wrote:
> > > 
> > > > +NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
> > > > +                            const char *alias);
> > > > +bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
> > > > +                               const char *alias);
> > > > +DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
> > > > +                                    const char *alias);
> > > 
> > > Could we have doc comments that document the purpose and API
> > > for these new global functions, please?
> > 
> > Like this? I deliberately fatfingered the argument names and didn't
> > even get a build warning, and I don't see any actual *documentation*
> > being generated with it...?
> 
> We use the doc comment format to allow for potential future
> documentation generation, but it's only actually generated
> if there's a .rst file somewhere under docs/ that has a
> kernel-doc:: directive referencing the .h file (for instance
> there's one in docs/devel/memory.rst that results in
> https://www.qemu.org/docs/master/devel/memory.html#api-reference )
> 
> For almost all internal functions, we set the relatively low
> bar of "have a doc comment so people reading the header file
> can see what the functions do". Where there's a more complex
> subsystem that merits its own hand-written documentation
> under docs/devel, then if the author of that documentation
> is enthusiastic they can clean up and pull in specific headers
> to add autogenerated docs. But the primary audience is the
> human reader of the .h file.

Ack, thanks.

> > diff --git a/include/net/net.h b/include/net/net.h
> > index 25ea83fd12..14614b0a31 100644
> > --- a/include/net/net.h
> > +++ b/include/net/net.h
> > @@ -207,10 +207,46 @@ int qemu_show_nic_models(const char *arg, const char *const *models);
> >  void qemu_check_nic_model(NICInfo *nd, const char *model);
> >  int qemu_find_nic_model(NICInfo *nd, const char * const *models,
> >                          const char *default_model);
> > +/**
> > + * qemu_find_nic_info: Obtain NIC configuration information
> > + * @typename: Name of device object type
> > + * @match_default: Match NIC configurations with no model specified
> > + * @alias: Additional model string to match (for user convenience and
> > + *         backward compatibility).
> > + *
> > + * Search for a NIC configuration matching the NIC model constraints.
> > + */
> >  NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
> >                              const char *alias);
> > +/**
> > + * qemu_configure_nic_device: Apply NIC configuration to a given device
> > + * @dev: Network device to be configured
> > + * @match_default: Match NIC configurations with no model specified
> > + * @alias: Additional model string to match
> > + *
> > + * Search for a NIC configuration for the provided device, using the
> > + * additionally specified matching constraints. If found, apply the
> > + * configuration using qdev_set_nic_properties() and return %true.
> > + *
> > + * This is used by platform code which creates the device anyway,
> > + * regardless of whether there is a configuration for it. This tends
> > + * to be platforms which ignore `--nodefaults` and create net devices
> > + * anyway. This behaviour is not advised for new platforms; use the
> > + * qemu_create_nic_device() function instead, which creates the device
> > + * only when it is configured.
> 
> I disagree about this paragraph. The behaviour we want for new
> platforms is:
> 
>  * If this is modelling some board where the ethernet device is
>    always present (eg it is soldered on to the board, or it is
>    a part of the SoC that the board uses), then always create
>    that device
>  * If the hardware being modelled has the ethernet device as an
>    optional device (eg physically removable like a PCI card),
>    then the board should arrange that --nodefaults causes it to
>    not be created
> 

Ack. Scratch the 'better behaved' part of my last response to Thomas
about smc91c111 too then :)

How's this:

/**
 * qemu_configure_nic_device: Apply NIC configuration to a given device
 * @dev: Network device to be configured
 * @match_default: Match NIC configurations with no model specified
 * @alias: Additional model string to match
 *
 * Search for a NIC configuration for the provided device, using the
 * additionally specified matching constraints. If found, apply the
 * configuration using qdev_set_nic_properties() and return %true.
 *
 * This is used by platform code which creates the device anyway,
 * regardless of whether there is a configuration for it. This tends
 * to be platforms which ignore `--nodefaults` and create net devices
 * anyway, for example because the Ethernet device on that board is
 * always physically present.
 */

On 08/01/2024 21.26, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> 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.
> 
> Also export the qemu_find_nic_info() helper, as some platforms have
> special cases they need to handle.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Paul Durrant <paul@xen.org>
> ---
>   include/net/net.h |  7 ++++++-
>   net/net.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/net.h b/include/net/net.h
> index ffbd2c8d56..25ea83fd12 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -207,7 +207,12 @@ int qemu_show_nic_models(const char *arg, const char *const *models);
>   void qemu_check_nic_model(NICInfo *nd, const char *model);
>   int qemu_find_nic_model(NICInfo *nd, const char * const *models,
>                           const char *default_model);
> -
> +NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
> +                            const char *alias);
> +bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
> +                               const char *alias);
> +DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
> +                                    const char *alias);
>   void print_net_client(Monitor *mon, NetClientState *nc);
>   void net_socket_rs_init(SocketReadState *rs,
>                           SocketReadStateFinalize *finalize,
> diff --git a/net/net.c b/net/net.c
> index 0520bc1681..aeb7f573fc 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1087,6 +1087,57 @@ static int net_init_nic(const Netdev *netdev, const char *name,
>       return idx;
>   }
>   
> +NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
> +                            const char *alias)
> +{
> +    NICInfo *nd;
> +    int i;
> +
> +    for (i = 0; i < nb_nics; i++) {
> +        nd = &nd_table[i];
> +
> +        if (!nd->used || nd->instantiated) {
> +            continue;
> +        }
> +
> +        if ((match_default && !nd->model) || !g_strcmp0(nd->model, typename)
> +            || (alias && !g_strcmp0(nd->model, alias))) {
> +            return nd;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +
> +/* "I have created a device. Please configure it if you can" */
> +bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
> +                               const char *alias)
> +{
> +    NICInfo *nd = qemu_find_nic_info(object_get_typename(OBJECT(dev)),
> +                                     match_default, alias);
> +
> +    if (nd) {
> +        qdev_set_nic_properties(dev, nd);
> +        return true;
> +    }
> +    return false;
> +}
> +
> +/* "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?

(same question likely applies to qemu_configure_nic_device() )

Apart from that, the patch looks fine to me.

  Thomas

> +    dev = qdev_new(typename);
> +    qdev_set_nic_properties(dev, nd);
> +    return dev;
> +}
>   
>   static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>       const Netdev *netdev,

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.


diff --git a/include/net/net.h b/include/net/net.h
index 766201c62c..ad6cd5b14b 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -238,7 +238,7 @@ GPtrArray *qemu_get_nic_models(const char *device_type);
 
 struct NICInfo {
     MACAddr macaddr;
-    char *model;
+    char *modelname;
     char *name;
     char *devaddr;
     NetClientState *netdev;
diff --git a/net/net.c b/net/net.c
index 71cccb19da..ab6185b4df 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1012,7 +1012,7 @@ static int net_init_nic(const Netdev *netdev, const char *name,
     }
     nd->name = g_strdup(name);
     if (nic->model) {
-        nd->model = g_strdup(nic->model);
+        nd->modelname = g_strdup(nic->model);
     }
     if (nic->addr) {
         nd->devaddr = g_strdup(nic->addr);
@@ -1142,8 +1142,8 @@ NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
             continue;
         }
 
-        if ((match_default && !nd->model) || !g_strcmp0(nd->model, typename)
-            || (alias && !g_strcmp0(nd->model, alias))) {
+        if ((match_default && !nd->modelname) || !g_strcmp0(nd->modelname, typename)
+            || (alias && !g_strcmp0(nd->modelname, alias))) {
             return nd;
         }
     }
@@ -1210,7 +1210,7 @@ void qemu_create_nic_bus_devices(BusState *bus, const char *parent_type,
             continue;
         }
 
-        model = nd->model ? nd->model : default_model;
+        model = nd->modelname ? nd->modelname : default_model;
         if (!model) {
             continue;
         }
@@ -1726,7 +1726,7 @@ void net_check_clients(void)
             warn_report("requested NIC (%s, model %s) "
                         "was not created (not supported by this machine?)",
                         nd->name ? nd->name : "anonymous",
-                        nd->model ? nd->model : "unspecified");
+                        nd->modelname ? nd->modelname : "unspecified");
         }
     }
 }
@@ -1787,9 +1787,9 @@ static int net_param_nic(void *dummy, QemuOpts *opts, Error **errp)
 
     ni = &nd_table[idx];
     memset(ni, 0, sizeof(*ni));
-    ni->model = qemu_opt_get_del(opts, "model");
+    ni->modelname = qemu_opt_get_del(opts, "model");
 
-    if (!nic_model_help && !g_strcmp0(ni->model, "help")) {
+    if (!nic_model_help && !g_strcmp0(ni->modelname, "help")) {
         nic_model_help = g_hash_table_new_full(g_str_hash, g_str_equal,
                                                g_free, NULL);
         return 0;

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?

  Thomas

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@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>

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@redhat.com>