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

[Thomas Huth <thuth@redhat.com>]

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,