Re: [PATCH 1/3] net: Move the code to collect available NIC models to a separate function

[Alex Bennée]

Thomas Huth <thuth@redhat.com> writes:

> The code that collects the available NIC models is not really specific
> to PCI anymore and will be required in the next patch, too, so let's
> move this into a new separate function in net.c instead.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/net/net.h |  1 +
>  hw/pci/pci.c      | 29 +----------------------------
>  net/net.c         | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 28 deletions(-)
>
> diff --git a/include/net/net.h b/include/net/net.h
> index 3db75ff841..c96cefb89a 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -189,6 +189,7 @@ void qemu_set_vnet_hdr_len(NetClientState *nc, int len);
>  int qemu_set_vnet_le(NetClientState *nc, bool is_le);
>  int qemu_set_vnet_be(NetClientState *nc, bool is_be);
>  void qemu_macaddr_default_if_unset(MACAddr *macaddr);
> +GPtrArray *qemu_get_nic_models(const char *device_type);
>  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,
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 2f450f6a72..2b7b343e82 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1964,7 +1964,6 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus 
> *rootbus,
>                                 const char *default_devaddr)
>  {
>      const char *devaddr = nd->devaddr ? nd->devaddr : default_devaddr;
> -    GSList *list;
>      GPtrArray *pci_nic_models;
>      PCIBus *bus;
>      PCIDevice *pci_dev;
> @@ -1979,33 +1978,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus 
> *rootbus,
>          nd->model = g_strdup("virtio-net-pci");
>      }
>  
> -    list = object_class_get_list_sorted(TYPE_PCI_DEVICE, false);
> -    pci_nic_models = g_ptr_array_new();
> -    while (list) {
> -        DeviceClass *dc = OBJECT_CLASS_CHECK(DeviceClass, list->data,
> -                                             TYPE_DEVICE);
> -        GSList *next;
> -        if (test_bit(DEVICE_CATEGORY_NETWORK, dc->categories) &&
> -            dc->user_creatable) {
> -            const char *name = object_class_get_name(list->data);
> -            /*
> -             * A network device might also be something else than a NIC, see
> -             * e.g. the "rocker" device. Thus we have to look for the 
> "netdev"
> -             * property, too. Unfortunately, some devices like virtio-net 
> only
> -             * create this property during instance_init, so we have to 
> create
> -             * a temporary instance here to be able to check it.
> -             */
> -            Object *obj = object_new_with_class(OBJECT_CLASS(dc));
> -            if (object_property_find(obj, "netdev")) {
> -                g_ptr_array_add(pci_nic_models, (gpointer)name);
> -            }
> -            object_unref(obj);
> -        }
> -        next = list->next;
> -        g_slist_free_1(list);
> -        list = next;
> -    }
> -    g_ptr_array_add(pci_nic_models, NULL);
> +    pci_nic_models = qemu_get_nic_models(TYPE_PCI_DEVICE);
>  
>      if (qemu_show_nic_models(nd->model, (const char 
> **)pci_nic_models->pdata)) {
>          exit(0);
> diff --git a/net/net.c b/net/net.c
> index 840ad9dca5..c0516a8067 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -899,6 +899,42 @@ static int nic_get_free_idx(void)
>      return -1;
>  }
>  
> +GPtrArray *qemu_get_nic_models(const char *device_type)
> +{
> +    GPtrArray *nic_models;
> +    GSList *list;
> +
> +    list = object_class_get_list_sorted(device_type, false);
> +    nic_models = g_ptr_array_new();
> +    while (list) {
> +        DeviceClass *dc = OBJECT_CLASS_CHECK(DeviceClass, list->data,
> +                                             TYPE_DEVICE);
> +        GSList *next;
> +        if (test_bit(DEVICE_CATEGORY_NETWORK, dc->categories) &&
> +            dc->user_creatable) {
> +            const char *name = object_class_get_name(list->data);
> +            /*
> +             * A network device might also be something else than a NIC, see
> +             * e.g. the "rocker" device. Thus we have to look for the 
> "netdev"
> +             * property, too. Unfortunately, some devices like virtio-net 
> only
> +             * create this property during instance_init, so we have to 
> create
> +             * a temporary instance here to be able to check it.
> +             */
> +            Object *obj = object_new_with_class(OBJECT_CLASS(dc));
> +            if (object_property_find(obj, "netdev")) {
> +                g_ptr_array_add(nic_models, (gpointer)name);
> +            }
> +            object_unref(obj);
> +        }
> +        next = list->next;
> +        g_slist_free_1(list);
> +        list = next;
> +    }
> +    g_ptr_array_add(nic_models, NULL);
> +
> +    return nic_models;
> +}

Is it worth freeing as you go and playing the next/list dance when you
could just:

  GPtrArray *qemu_get_nic_models(const char *device_type)
  {
      GPtrArray *nic_models = g_ptr_array_new();
      g_autoptr(GSList) list = object_class_get_list_sorted(device_type, false);

      do {
          DeviceClass *dc = OBJECT_CLASS_CHECK(DeviceClass, list->data,
                                               TYPE_DEVICE);
          if (test_bit(DEVICE_CATEGORY_NETWORK, dc->categories) &&
              dc->user_creatable) {
              const char *name = object_class_get_name(list->data);
              /*
               * A network device might also be something else than a NIC, see
               * e.g. the "rocker" device. Thus we have to look for the "netdev"
               * property, too. Unfortunately, some devices like virtio-net only
               * create this property during instance_init, so we have to create
               * a temporary instance here to be able to check it.
               */
              Object *obj = object_new_with_class(OBJECT_CLASS(dc));
              if (object_property_find(obj, "netdev")) {
                  g_ptr_array_add(nic_models, (gpointer)name);
              }
              object_unref(obj);
          }
      } while ((list = g_slist_next(list)));
      g_ptr_array_add(nic_models, NULL);

      return nic_models;
  }

I must admit I'm not super clear on the lifetimes
object_class_get_list_sorted but I assume the contents are static and we
only need the equivalent of g_slist_free. 


> +
>  int qemu_show_nic_models(const char *arg, const char *const *models)
>  {
>      int i;


-- 
Alex Bennée