[PATCH 0/3] Fix the "-nic help" option

[PATCH 0/3] Fix the "-nic help" option

[Thomas Huth]

Running QEMU with "-nic help" used to work in QEMU 5.2 and earlier
versions, but since QEMU 6.0 it just complains that "help" is not
a valid value here. This patch series fixes this problem and also
extends the help output here to list the available NIC models, too.

Thomas Huth (3):
  net: Move the code to collect available NIC models to a separate
    function
  net: Restore printing of the help text with "-nic help"
  net: Replace "Supported NIC models" with "Available NIC models"

 include/net/net.h |  1 +
 hw/pci/pci.c      | 29 +-------------------------
 net/net.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 51 insertions(+), 31 deletions(-)

-- 
2.31.1

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

[Thomas Huth]

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;
+}
+
 int qemu_show_nic_models(const char *arg, const char *const *models)
 {
     int i;
-- 
2.31.1

[PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Thomas Huth]

Running QEMU with "-nic help" used to work in QEMU 5.2 and earlier versions
(it showed the available netdev backends), but this feature got broken during
some refactoring in version 6.0. Let's restore the old behavior, and while
we're at it, let's also print the available NIC models here now since this
option can be used to configure both, netdev backend and model in one go.

Fixes: ad6f932fe8 ("net: do not exit on "netdev_add help" monitor command")
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 net/net.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/net.c b/net/net.c
index c0516a8067..b4b8f2a9cc 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1571,8 +1571,18 @@ static int net_param_nic(void *dummy, QemuOpts *opts, Error **errp)
     const char *type;
 
     type = qemu_opt_get(opts, "type");
-    if (type && g_str_equal(type, "none")) {
-        return 0;    /* Nothing to do, default_net is cleared in vl.c */
+    if (type) {
+        if (g_str_equal(type, "none")) {
+            return 0;    /* Nothing to do, default_net is cleared in vl.c */
+        }
+        if (is_help_option(type)) {
+            GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
+            show_netdevs();
+            printf("\n");
+            qemu_show_nic_models(type, (const char **)nic_models->pdata);
+            g_ptr_array_free(nic_models, true);
+            exit(0);
+        }
     }
 
     idx = nic_get_free_idx();
-- 
2.31.1

[PATCH 3/3] net: Replace "Supported NIC models" with "Available NIC models"

[Thomas Huth]

Just because a NIC model is compiled into the QEMU binary does not
necessary mean that it can be used with each and every machine.
So let's rather talk about "available" models instead of "supported"
models, just to avoid confusion.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 net/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index b4b8f2a9cc..173195dbf9 100644
--- a/net/net.c
+++ b/net/net.c
@@ -943,7 +943,7 @@ int qemu_show_nic_models(const char *arg, const char *const *models)
         return 0;
     }
 
-    printf("Supported NIC models:\n");
+    printf("Available NIC models:\n");
     for (i = 0 ; models[i]; i++) {
         printf("%s\n", models[i]);
     }
-- 
2.31.1

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

[Claudio Fontana]

On 11/4/22 13:57, Thomas Huth wrote:
> 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);

I know there is no precedent in this file, but it would be useful to document this function,
what it does exactly and what it returns, the return value, allocation assumptions etc.

>  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;
> +}
> +
>  int qemu_show_nic_models(const char *arg, const char *const *models)
>  {
>      int i;

Claudio

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Claudio Fontana]

should -net and -netdev be adapted too?

For audio, we now have support for help options in both -audiodev and -audio..

Thanks,

Claudio

On 11/4/22 13:57, Thomas Huth wrote:
> Running QEMU with "-nic help" used to work in QEMU 5.2 and earlier versions
> (it showed the available netdev backends), but this feature got broken during
> some refactoring in version 6.0. Let's restore the old behavior, and while
> we're at it, let's also print the available NIC models here now since this
> option can be used to configure both, netdev backend and model in one go.
> 
> Fixes: ad6f932fe8 ("net: do not exit on "netdev_add help" monitor command")
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  net/net.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/net.c b/net/net.c
> index c0516a8067..b4b8f2a9cc 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1571,8 +1571,18 @@ static int net_param_nic(void *dummy, QemuOpts *opts, Error **errp)
>      const char *type;
>  
>      type = qemu_opt_get(opts, "type");
> -    if (type && g_str_equal(type, "none")) {
> -        return 0;    /* Nothing to do, default_net is cleared in vl.c */
> +    if (type) {
> +        if (g_str_equal(type, "none")) {
> +            return 0;    /* Nothing to do, default_net is cleared in vl.c */
> +        }
> +        if (is_help_option(type)) {
> +            GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
> +            show_netdevs();
> +            printf("\n");
> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
> +            g_ptr_array_free(nic_models, true);
> +            exit(0);
> +        }
>      }
>  
>      idx = nic_get_free_idx();

Re: [PATCH 3/3] net: Replace "Supported NIC models" with "Available NIC models"

[Claudio Fontana]

On 11/4/22 13:57, Thomas Huth wrote:
> Just because a NIC model is compiled into the QEMU binary does not
> necessary mean that it can be used with each and every machine.
> So let's rather talk about "available" models instead of "supported"
> models, just to avoid confusion.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  net/net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/net.c b/net/net.c
> index b4b8f2a9cc..173195dbf9 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -943,7 +943,7 @@ int qemu_show_nic_models(const char *arg, const char *const *models)
>          return 0;
>      }
>  
> -    printf("Supported NIC models:\n");
> +    printf("Available NIC models:\n");
>      for (i = 0 ; models[i]; i++) {
>          printf("%s\n", models[i]);
>      }

Reviewed-by: Claudio Fontana <cfontana@suse.de>

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

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Thomas Huth]

On 07/11/2022 13.27, Claudio Fontana wrote:
> should -net and -netdev be adapted too?

"-netdev help" already works just fine ... and "-net" should IMHO rather be 
removed than improved ;-)

  Thomas

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Claudio Fontana]

On 11/8/22 09:42, Thomas Huth wrote:
> On 07/11/2022 13.27, Claudio Fontana wrote:
>> should -net and -netdev be adapted too?
> 
> "-netdev help" already works just fine ... and "-net" should IMHO rather be 
> removed than improved ;-)
> 
>   Thomas
> 

I wonder if it could be done once for all, in net_init_clients,
instead of repeating the is_help_option in net_init_netdev and net_param_nic
(and that would take care of net_init_client too, so we'd get "net" for free)..

Ciao,

C
 

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Thomas Huth]

On 08/11/2022 09.52, Claudio Fontana wrote:
> On 11/8/22 09:42, Thomas Huth wrote:
>> On 07/11/2022 13.27, Claudio Fontana wrote:
>>> should -net and -netdev be adapted too?
>>
>> "-netdev help" already works just fine ... and "-net" should IMHO rather be
>> removed than improved ;-)
>>
>>    Thomas
>>
> 
> I wonder if it could be done once for all, in net_init_clients,
> instead of repeating the is_help_option in net_init_netdev and net_param_nic
> (and that would take care of net_init_client too, so we'd get "net" for free)..

I don't think that it makes too much sense to have one option for all - 
since all three CLI options are slightly different anyway. E.g. "-net nic" 
only exists for "-net", "hubport" cannot be used with "-net", "-nic" can 
also be used to configure the NIC model, etc.

  Thomas

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Claudio Fontana]

On 11/8/22 09:59, Thomas Huth wrote:
> On 08/11/2022 09.52, Claudio Fontana wrote:
>> On 11/8/22 09:42, Thomas Huth wrote:
>>> On 07/11/2022 13.27, Claudio Fontana wrote:
>>>> should -net and -netdev be adapted too?
>>>
>>> "-netdev help" already works just fine ... and "-net" should IMHO rather be
>>> removed than improved ;-)
>>>
>>>    Thomas
>>>
>>
>> I wonder if it could be done once for all, in net_init_clients,
>> instead of repeating the is_help_option in net_init_netdev and net_param_nic
>> (and that would take care of net_init_client too, so we'd get "net" for free)..
> 
> I don't think that it makes too much sense to have one option for all - 
> since all three CLI options are slightly different anyway. E.g. "-net nic" 
> only exists for "-net", "hubport" cannot be used with "-net", "-nic" can 
> also be used to configure the NIC model, etc.
> 
>   Thomas
> 

Hi Thomas, I would not suggest to merge everything together,

I was considering whether it would make sense to just check the "type" id for is_help_option
once, since all the options "net", "netdev", "nic" have a "type" implied_opt_name,

and so it should be possible to make a list of structs that signify what to do for "net", "netdev", "nic", and 
loop on that and check for that help string once,

and then split off the codepath into the "net", "netdev", "nic" - specific code as it is now,
either manually or by storing the function that is now in the foreach as a function pointer in the struct, ie moving the is_help_option check one level up.

However, it might not be worth it since it seems that for "nic" the nic models need to also be printed, so it might make things needlessly verbose.

Not sure, have not tried to write the code for it.

Ciao,

Claudio

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Claudio Fontana]

On 11/4/22 13:57, Thomas Huth wrote:
> Running QEMU with "-nic help" used to work in QEMU 5.2 and earlier versions
> (it showed the available netdev backends), but this feature got broken during
> some refactoring in version 6.0. Let's restore the old behavior, and while
> we're at it, let's also print the available NIC models here now since this
> option can be used to configure both, netdev backend and model in one go.
> 
> Fixes: ad6f932fe8 ("net: do not exit on "netdev_add help" monitor command")
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  net/net.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/net.c b/net/net.c
> index c0516a8067..b4b8f2a9cc 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1571,8 +1571,18 @@ static int net_param_nic(void *dummy, QemuOpts *opts, Error **errp)
>      const char *type;
>  
>      type = qemu_opt_get(opts, "type");
> -    if (type && g_str_equal(type, "none")) {
> -        return 0;    /* Nothing to do, default_net is cleared in vl.c */
> +    if (type) {
> +        if (g_str_equal(type, "none")) {
> +            return 0;    /* Nothing to do, default_net is cleared in vl.c */
> +        }
> +        if (is_help_option(type)) {
> +            GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
> +            show_netdevs();
> +            printf("\n");
> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
> +            g_ptr_array_free(nic_models, true);

nit: would not the order:

> +            GPtrArray *nic_models;
> +            show_netdevs();
> +            printf("\n");
> +            nic_models = qemu_get_nic_models(TYPE_DEVICE);
> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
> +            g_ptr_array_free(nic_models, true);

flow more logically?

Ciao

C

> +            exit(0);
> +        }
>      }
>  
>      idx = nic_get_free_idx();

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

[Thomas Huth]

On 07/11/2022 18.34, Alex Bennée wrote:
> 
> 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.

Looks like it could work, too. I'll add a patch on top to change it.

  Thomas

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Thomas Huth]

On 08/11/2022 10.43, Claudio Fontana wrote:
> On 11/8/22 09:59, Thomas Huth wrote:
>> On 08/11/2022 09.52, Claudio Fontana wrote:
>>> On 11/8/22 09:42, Thomas Huth wrote:
>>>> On 07/11/2022 13.27, Claudio Fontana wrote:
>>>>> should -net and -netdev be adapted too?
>>>>
>>>> "-netdev help" already works just fine ... and "-net" should IMHO rather be
>>>> removed than improved ;-)
>>>>
>>>>     Thomas
>>>>
>>>
>>> I wonder if it could be done once for all, in net_init_clients,
>>> instead of repeating the is_help_option in net_init_netdev and net_param_nic
>>> (and that would take care of net_init_client too, so we'd get "net" for free)..
>>
>> I don't think that it makes too much sense to have one option for all -
>> since all three CLI options are slightly different anyway. E.g. "-net nic"
>> only exists for "-net", "hubport" cannot be used with "-net", "-nic" can
>> also be used to configure the NIC model, etc.
>>
>>    Thomas
>>
> 
> Hi Thomas, I would not suggest to merge everything together,
> 
> I was considering whether it would make sense to just check the "type" id for is_help_option
> once, since all the options "net", "netdev", "nic" have a "type" implied_opt_name,
> 
> and so it should be possible to make a list of structs that signify what to do for "net", "netdev", "nic", and
> loop on that and check for that help string once,
> 
> and then split off the codepath into the "net", "netdev", "nic" - specific code as it is now,
> either manually or by storing the function that is now in the foreach as a function pointer in the struct, ie moving the is_help_option check one level up.
> 
> However, it might not be worth it since it seems that for "nic" the nic models need to also be printed, so it might make things needlessly verbose.
> 
> Not sure, have not tried to write the code for it.

Sorry, I currently fail to see how that should really work out nicely, so 
I'll continue with my current patch. But feel free to write some patches, 
maybe it's clearer to me if I see the code.

  Thomas

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Thomas Huth]

On 08/11/2022 10.49, Claudio Fontana wrote:
> On 11/4/22 13:57, Thomas Huth wrote:
>> Running QEMU with "-nic help" used to work in QEMU 5.2 and earlier versions
>> (it showed the available netdev backends), but this feature got broken during
>> some refactoring in version 6.0. Let's restore the old behavior, and while
>> we're at it, let's also print the available NIC models here now since this
>> option can be used to configure both, netdev backend and model in one go.
>>
>> Fixes: ad6f932fe8 ("net: do not exit on "netdev_add help" monitor command")
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   net/net.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/net.c b/net/net.c
>> index c0516a8067..b4b8f2a9cc 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -1571,8 +1571,18 @@ static int net_param_nic(void *dummy, QemuOpts *opts, Error **errp)
>>       const char *type;
>>   
>>       type = qemu_opt_get(opts, "type");
>> -    if (type && g_str_equal(type, "none")) {
>> -        return 0;    /* Nothing to do, default_net is cleared in vl.c */
>> +    if (type) {
>> +        if (g_str_equal(type, "none")) {
>> +            return 0;    /* Nothing to do, default_net is cleared in vl.c */
>> +        }
>> +        if (is_help_option(type)) {
>> +            GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
>> +            show_netdevs();
>> +            printf("\n");
>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>> +            g_ptr_array_free(nic_models, true);
> 
> nit: would not the order:
> 
>> +            GPtrArray *nic_models;
>> +            show_netdevs();
>> +            printf("\n");
>> +            nic_models = qemu_get_nic_models(TYPE_DEVICE);
>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>> +            g_ptr_array_free(nic_models, true);
> 
> flow more logically?

I think that's mostly a matter of taste ... and as long as the declaration 
of the variable has to stay at the top of the block (according to QEMU's 
coding style), I think I'd also prefer to keep the initialization there.

  Thomas

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Claudio Fontana]

On 11/10/22 12:35, Thomas Huth wrote:
> On 08/11/2022 10.43, Claudio Fontana wrote:
>> On 11/8/22 09:59, Thomas Huth wrote:
>>> On 08/11/2022 09.52, Claudio Fontana wrote:
>>>> On 11/8/22 09:42, Thomas Huth wrote:
>>>>> On 07/11/2022 13.27, Claudio Fontana wrote:
>>>>>> should -net and -netdev be adapted too?
>>>>>
>>>>> "-netdev help" already works just fine ... and "-net" should IMHO rather be
>>>>> removed than improved ;-)
>>>>>
>>>>>     Thomas
>>>>>
>>>>
>>>> I wonder if it could be done once for all, in net_init_clients,
>>>> instead of repeating the is_help_option in net_init_netdev and net_param_nic
>>>> (and that would take care of net_init_client too, so we'd get "net" for free)..
>>>
>>> I don't think that it makes too much sense to have one option for all -
>>> since all three CLI options are slightly different anyway. E.g. "-net nic"
>>> only exists for "-net", "hubport" cannot be used with "-net", "-nic" can
>>> also be used to configure the NIC model, etc.
>>>
>>>    Thomas
>>>
>>
>> Hi Thomas, I would not suggest to merge everything together,
>>
>> I was considering whether it would make sense to just check the "type" id for is_help_option
>> once, since all the options "net", "netdev", "nic" have a "type" implied_opt_name,
>>
>> and so it should be possible to make a list of structs that signify what to do for "net", "netdev", "nic", and
>> loop on that and check for that help string once,
>>
>> and then split off the codepath into the "net", "netdev", "nic" - specific code as it is now,
>> either manually or by storing the function that is now in the foreach as a function pointer in the struct, ie moving the is_help_option check one level up.
>>
>> However, it might not be worth it since it seems that for "nic" the nic models need to also be printed, so it might make things needlessly verbose.
>>
>> Not sure, have not tried to write the code for it.
> 
> Sorry, I currently fail to see how that should really work out nicely, so 
> I'll continue with my current patch. But feel free to write some patches, 
> maybe it's clearer to me if I see the code.
> 
>   Thomas
> 

No worries, maybe it's just too complex and not worth it.

Ciao,

C

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Claudio Fontana]

On 11/10/22 12:42, Thomas Huth wrote:
> On 08/11/2022 10.49, Claudio Fontana wrote:
>> On 11/4/22 13:57, Thomas Huth wrote:
>>> Running QEMU with "-nic help" used to work in QEMU 5.2 and earlier versions
>>> (it showed the available netdev backends), but this feature got broken during
>>> some refactoring in version 6.0. Let's restore the old behavior, and while
>>> we're at it, let's also print the available NIC models here now since this
>>> option can be used to configure both, netdev backend and model in one go.
>>>
>>> Fixes: ad6f932fe8 ("net: do not exit on "netdev_add help" monitor command")
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   net/net.c | 14 ++++++++++++--
>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/net.c b/net/net.c
>>> index c0516a8067..b4b8f2a9cc 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -1571,8 +1571,18 @@ static int net_param_nic(void *dummy, QemuOpts *opts, Error **errp)
>>>       const char *type;
>>>   
>>>       type = qemu_opt_get(opts, "type");
>>> -    if (type && g_str_equal(type, "none")) {
>>> -        return 0;    /* Nothing to do, default_net is cleared in vl.c */
>>> +    if (type) {
>>> +        if (g_str_equal(type, "none")) {
>>> +            return 0;    /* Nothing to do, default_net is cleared in vl.c */
>>> +        }
>>> +        if (is_help_option(type)) {
>>> +            GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
>>> +            show_netdevs();
>>> +            printf("\n");
>>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>>> +            g_ptr_array_free(nic_models, true);
>>
>> nit: would not the order:
>>
>>> +            GPtrArray *nic_models;
>>> +            show_netdevs();
>>> +            printf("\n");
>>> +            nic_models = qemu_get_nic_models(TYPE_DEVICE);
>>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>>> +            g_ptr_array_free(nic_models, true);
>>
>> flow more logically?
> 
> I think that's mostly a matter of taste ...

To some extent, but for the reader it would make more sense not to intermix unrelated code?

I'd say:

- show_netdevs
_ get nic models
- show nic models

instead of:

- get nic models
- show netdevs
- show nic models


 > and as long as the declaration 
> of the variable has to stay at the top of the block (according to QEMU's 
> coding style), I think I'd also prefer to keep the initialization there.
> 

This conflict can easily be solved by putting the nic_models on its own line,
as I have shown in my hunk above.

Ciao,

Claudio

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Claudio Fontana]

On 11/10/22 13:05, Claudio Fontana wrote:
> On 11/10/22 12:42, Thomas Huth wrote:
>> On 08/11/2022 10.49, Claudio Fontana wrote:
>>> On 11/4/22 13:57, Thomas Huth wrote:
>>>> Running QEMU with "-nic help" used to work in QEMU 5.2 and earlier versions
>>>> (it showed the available netdev backends), but this feature got broken during
>>>> some refactoring in version 6.0. Let's restore the old behavior, and while
>>>> we're at it, let's also print the available NIC models here now since this
>>>> option can be used to configure both, netdev backend and model in one go.
>>>>
>>>> Fixes: ad6f932fe8 ("net: do not exit on "netdev_add help" monitor command")
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>   net/net.c | 14 ++++++++++++--
>>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/net.c b/net/net.c
>>>> index c0516a8067..b4b8f2a9cc 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -1571,8 +1571,18 @@ static int net_param_nic(void *dummy, QemuOpts *opts, Error **errp)
>>>>       const char *type;
>>>>   
>>>>       type = qemu_opt_get(opts, "type");
>>>> -    if (type && g_str_equal(type, "none")) {
>>>> -        return 0;    /* Nothing to do, default_net is cleared in vl.c */
>>>> +    if (type) {
>>>> +        if (g_str_equal(type, "none")) {
>>>> +            return 0;    /* Nothing to do, default_net is cleared in vl.c */
>>>> +        }
>>>> +        if (is_help_option(type)) {
>>>> +            GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
>>>> +            show_netdevs();
>>>> +            printf("\n");
>>>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>>>> +            g_ptr_array_free(nic_models, true);
>>>
>>> nit: would not the order:
>>>
>>>> +            GPtrArray *nic_models;
>>>> +            show_netdevs();
>>>> +            printf("\n");
>>>> +            nic_models = qemu_get_nic_models(TYPE_DEVICE);
>>>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>>>> +            g_ptr_array_free(nic_models, true);
>>>
>>> flow more logically?
>>
>> I think that's mostly a matter of taste ...
> 
> To some extent, but for the reader it would make more sense not to intermix unrelated code?
> 
> I'd say:
> 
> - show_netdevs
> _ get nic models
> - show nic models
> 
> instead of:
> 
> - get nic models
> - show netdevs
> - show nic models
> 
> 
>  > and as long as the declaration 
>> of the variable has to stay at the top of the block (according to QEMU's 
>> coding style), I think I'd also prefer to keep the initialization there.
>>
> 
> This conflict can easily be solved by putting the nic_models on its own line,
> as I have shown in my hunk above.
>

And another option (not used enough in QEMU code imo), is to open a block specifically for the nic models.

Ie you can say:

 show_netdevs();
 printf("\n");
 {
     GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
     qemu_show_nic_models(type, (const char **)nic_models->pdata);
     g_ptr_array_free(nic_models, true);
 }

this even has the additional benefit of making the allocation/free of the nic_models clearer as it corresponds to the block.

C

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Thomas Huth]

On 10/11/2022 13.05, Claudio Fontana wrote:
> On 11/10/22 12:42, Thomas Huth wrote:
>> On 08/11/2022 10.49, Claudio Fontana wrote:
>>> On 11/4/22 13:57, Thomas Huth wrote:
>>>> Running QEMU with "-nic help" used to work in QEMU 5.2 and earlier versions
>>>> (it showed the available netdev backends), but this feature got broken during
>>>> some refactoring in version 6.0. Let's restore the old behavior, and while
>>>> we're at it, let's also print the available NIC models here now since this
>>>> option can be used to configure both, netdev backend and model in one go.
>>>>
>>>> Fixes: ad6f932fe8 ("net: do not exit on "netdev_add help" monitor command")
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    net/net.c | 14 ++++++++++++--
>>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/net.c b/net/net.c
>>>> index c0516a8067..b4b8f2a9cc 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -1571,8 +1571,18 @@ static int net_param_nic(void *dummy, QemuOpts *opts, Error **errp)
>>>>        const char *type;
>>>>    
>>>>        type = qemu_opt_get(opts, "type");
>>>> -    if (type && g_str_equal(type, "none")) {
>>>> -        return 0;    /* Nothing to do, default_net is cleared in vl.c */
>>>> +    if (type) {
>>>> +        if (g_str_equal(type, "none")) {
>>>> +            return 0;    /* Nothing to do, default_net is cleared in vl.c */
>>>> +        }
>>>> +        if (is_help_option(type)) {
>>>> +            GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
>>>> +            show_netdevs();
>>>> +            printf("\n");
>>>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>>>> +            g_ptr_array_free(nic_models, true);
>>>
>>> nit: would not the order:
>>>
>>>> +            GPtrArray *nic_models;
>>>> +            show_netdevs();
>>>> +            printf("\n");
>>>> +            nic_models = qemu_get_nic_models(TYPE_DEVICE);
>>>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>>>> +            g_ptr_array_free(nic_models, true);
>>>
>>> flow more logically?
>>
>> I think that's mostly a matter of taste ...
> 
> To some extent, but for the reader it would make more sense not to intermix unrelated code?

I'm pretty sure that as soon as I change it, another reviewer
shows up and asks me to put everything into one line again
since they prefer more compact code ;-)

> I'd say:
> 
> - show_netdevs
> _ get nic models
> - show nic models
> 
> instead of:
> 
> - get nic models
> - show netdevs
> - show nic models

I get your point, and I would immediately agree with you if we
were allowed to do:

         show_netdevs();
         printf("\n");
         GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
         qemu_show_nic_models(type, (const char **)nic_models->pdata);
         g_ptr_array_free(nic_models, true);

Although this is possible nowadays (since we're using
not C89 anymore), it's against the QEMU coding style.

So it's a trade-off now - use two lines of code and have some more
chronological code flow, or use one line of more compact code.

I'm in favor of the more compact code. So please let's stop
bike-shedding now.

  Thanks,
   Thomas

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

[Thomas Huth]

On 10/11/2022 11.35, Thomas Huth wrote:
> On 07/11/2022 18.34, Alex Bennée wrote:
>>
>> 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.
> 
> Looks like it could work, too. I'll add a patch on top to change it.

Ok, now I've tried, and it does not work - valgrind compains about leaking 
memory here. The problem is: You have to keep the pointer to the list head 
around, by doing list = g_slist_next(list) you leave memory behind that 
cannot be freed anymore. Thus I'll drop this change for now, since keeping 
the pointer to the list head just for freeing it later is also ugly.

  Thomas

Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Claudio Fontana]

On 11/10/22 13:23, Thomas Huth wrote:
> On 10/11/2022 13.05, Claudio Fontana wrote:
>> On 11/10/22 12:42, Thomas Huth wrote:
>>> On 08/11/2022 10.49, Claudio Fontana wrote:
>>>> On 11/4/22 13:57, Thomas Huth wrote:
>>>>> Running QEMU with "-nic help" used to work in QEMU 5.2 and earlier versions
>>>>> (it showed the available netdev backends), but this feature got broken during
>>>>> some refactoring in version 6.0. Let's restore the old behavior, and while
>>>>> we're at it, let's also print the available NIC models here now since this
>>>>> option can be used to configure both, netdev backend and model in one go.
>>>>>
>>>>> Fixes: ad6f932fe8 ("net: do not exit on "netdev_add help" monitor command")
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>    net/net.c | 14 ++++++++++++--
>>>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/net/net.c b/net/net.c
>>>>> index c0516a8067..b4b8f2a9cc 100644
>>>>> --- a/net/net.c
>>>>> +++ b/net/net.c
>>>>> @@ -1571,8 +1571,18 @@ static int net_param_nic(void *dummy, QemuOpts *opts, Error **errp)
>>>>>        const char *type;
>>>>>    
>>>>>        type = qemu_opt_get(opts, "type");
>>>>> -    if (type && g_str_equal(type, "none")) {
>>>>> -        return 0;    /* Nothing to do, default_net is cleared in vl.c */
>>>>> +    if (type) {
>>>>> +        if (g_str_equal(type, "none")) {
>>>>> +            return 0;    /* Nothing to do, default_net is cleared in vl.c */
>>>>> +        }
>>>>> +        if (is_help_option(type)) {
>>>>> +            GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
>>>>> +            show_netdevs();
>>>>> +            printf("\n");
>>>>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>>>>> +            g_ptr_array_free(nic_models, true);
>>>>
>>>> nit: would not the order:
>>>>
>>>>> +            GPtrArray *nic_models;
>>>>> +            show_netdevs();
>>>>> +            printf("\n");
>>>>> +            nic_models = qemu_get_nic_models(TYPE_DEVICE);
>>>>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>>>>> +            g_ptr_array_free(nic_models, true);
>>>>
>>>> flow more logically?
>>>
>>> I think that's mostly a matter of taste ...
>>
>> To some extent, but for the reader it would make more sense not to intermix unrelated code?
> 
> I'm pretty sure that as soon as I change it, another reviewer
> shows up and asks me to put everything into one line again
> since they prefer more compact code ;-)
> 
>> I'd say:
>>
>> - show_netdevs
>> _ get nic models
>> - show nic models
>>
>> instead of:
>>
>> - get nic models
>> - show netdevs
>> - show nic models
> 
> I get your point, and I would immediately agree with you if we
> were allowed to do:
> 
>          show_netdevs();
>          printf("\n");
>          GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
>          qemu_show_nic_models(type, (const char **)nic_models->pdata);
>          g_ptr_array_free(nic_models, true);
> 
> Although this is possible nowadays (since we're using
> not C89 anymore), it's against the QEMU coding style.

It seems you haven't seen the next message I sent. You can write exactly the code above,
with valid C89 syntax,

which is:

show_netdevs();
printf("\n");
{
   GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
   qemu_show_nic_models(type, (const char **)nic_models->pdata);
   g_ptr_array_free(nic_models, true);
}

> 
> So it's a trade-off now - use two lines of code and have some more
> chronological code flow, or use one line of more compact code.
> 
> I'm in favor of the more compact code. So please let's stop
> bike-shedding now.
> 
>   Thanks,
>    Thomas
> 

Ah, you don't like it, I see. Well.

Thanks,

Claudio