| Message ID | 20180306194600.9650-3-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06.03.2018 20:45, Paolo Bonzini wrote: > Remove the hard-coded list of PCI NIC names; instead, fill an array > using all PCI devices listed under DEVICE_CATEGORY_NETWORK. Keep > the old shortcut "virtio" for virtio-net-pci. > > Suggested-by: Thomas Huth <thuth@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/pci/pci.c | 61 ++++++++++++++++++++++++++++++------------------------------ > 1 file changed, 30 insertions(+), 31 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 2174c254eb..aa24a26680 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1815,49 +1815,48 @@ PciInfoList *qmp_query_pci(Error **errp) > return head; > } > > -static const char * const pci_nic_models[] = { > - "ne2k_pci", > - "i82551", > - "i82557b", > - "i82559er", > - "rtl8139", > - "e1000", > - "pcnet", > - "virtio", > - "sungem", > - NULL > -}; > - > -static const char * const pci_nic_names[] = { > - "ne2k_pci", > - "i82551", > - "i82557b", > - "i82559er", > - "rtl8139", > - "e1000", > - "pcnet", > - "virtio-net-pci", > - "sungem", > - NULL > -}; > - > /* Initialize a PCI NIC. */ > PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus, > const char *default_model, > const char *default_devaddr) > { > const char *devaddr = nd->devaddr ? nd->devaddr : default_devaddr; > + GSList *list; > + GPtrArray *pci_nic_models; > PCIBus *bus; > PCIDevice *pci_dev; > DeviceState *dev; > int devfn; > int i; > > - if (qemu_show_nic_models(nd->model, pci_nic_models)) { > + if (!strcmp(nd->model, "virtio")) { > + g_free(nd->model); > + nd->model = g_strdup("virtio-net-pci"); Should we maybe also print out a deprecation message in this case, so that we could get rid of this hack in a couple of releases? (i.e. only allow "virtio-net-pci" in future releases?) > + } > + > + list = object_class_get_list_sorted(TYPE_PCI_DEVICE, false); > + pci_nic_models = g_ptr_array_new_with_free_func(g_free); OK, so that means that the entries will be freed when the array is destroyed, right? ... > + 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) ; ... but object_class_get_name() returns the original name, not a g_strdup of it, right? (apart from that: Please remove the space before the semicolon) > + g_ptr_array_add(pci_nic_models, (gpointer)name); > + } > + next = list->next; > + g_slist_free_1(list); > + list = next; > + } > + g_ptr_array_add(pci_nic_models, NULL); > + > + if (qemu_show_nic_models(nd->model, (const char **)pci_nic_models->pdata)) { > exit(0); > } > > - i = qemu_find_nic_model(nd, pci_nic_models, default_model); > + i = qemu_find_nic_model(nd, (const char **)pci_nic_models->pdata, > + default_model); > if (i < 0) { > exit(1); > } > @@ -1865,15 +1864,15 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus, > bus = pci_get_bus_devfn(&devfn, rootbus, devaddr); > if (!bus) { > error_report("Invalid PCI device address %s for device %s", > - devaddr, pci_nic_names[i]); > + devaddr, nd->model); > exit(1); > } > > - pci_dev = pci_create(bus, devfn, pci_nic_names[i]); > + pci_dev = pci_create(bus, devfn, nd->model); > dev = &pci_dev->qdev; > qdev_set_nic_properties(dev, nd); > qdev_init_nofail(dev); > - > + g_ptr_array_free(pci_nic_models, true); ... so this will g_free the original pointers of the device classes - I guess this will sooner or later cause some trouble. Apart from that, I really like this patch - that hard-coded pci_nic_models[] was really ugly. Thomas
On 08/03/2018 10:02, Thomas Huth wrote: > On 06.03.2018 20:45, Paolo Bonzini wrote: >> Remove the hard-coded list of PCI NIC names; instead, fill an array >> using all PCI devices listed under DEVICE_CATEGORY_NETWORK. Keep >> the old shortcut "virtio" for virtio-net-pci. >> >> Suggested-by: Thomas Huth <thuth@redhat.com> >> Cc: Jason Wang <jasowang@redhat.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> hw/pci/pci.c | 61 ++++++++++++++++++++++++++++++------------------------------ >> 1 file changed, 30 insertions(+), 31 deletions(-) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index 2174c254eb..aa24a26680 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -1815,49 +1815,48 @@ PciInfoList *qmp_query_pci(Error **errp) >> return head; >> } >> >> -static const char * const pci_nic_models[] = { >> - "ne2k_pci", >> - "i82551", >> - "i82557b", >> - "i82559er", >> - "rtl8139", >> - "e1000", >> - "pcnet", >> - "virtio", >> - "sungem", >> - NULL >> -}; >> - >> -static const char * const pci_nic_names[] = { >> - "ne2k_pci", >> - "i82551", >> - "i82557b", >> - "i82559er", >> - "rtl8139", >> - "e1000", >> - "pcnet", >> - "virtio-net-pci", >> - "sungem", >> - NULL >> -}; >> - >> /* Initialize a PCI NIC. */ >> PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus, >> const char *default_model, >> const char *default_devaddr) >> { >> const char *devaddr = nd->devaddr ? nd->devaddr : default_devaddr; >> + GSList *list; >> + GPtrArray *pci_nic_models; >> PCIBus *bus; >> PCIDevice *pci_dev; >> DeviceState *dev; >> int devfn; >> int i; >> >> - if (qemu_show_nic_models(nd->model, pci_nic_models)) { >> + if (!strcmp(nd->model, "virtio")) { >> + g_free(nd->model); >> + nd->model = g_strdup("virtio-net-pci"); > > Should we maybe also print out a deprecation message in this case, so > that we could get rid of this hack in a couple of releases? (i.e. only > allow "virtio-net-pci" in future releases?) I don't know... Like -net, this is probably present in countless tutorials/blog posts, and it would be very hard to remove it. >> + } >> + >> + list = object_class_get_list_sorted(TYPE_PCI_DEVICE, false); >> + pci_nic_models = g_ptr_array_new_with_free_func(g_free); > > OK, so that means that the entries will be freed when the array is > destroyed, right? ... Duh. valgrind isn't happy either. I'll send v3. :( Paolo
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 2174c254eb..aa24a26680 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1815,49 +1815,48 @@ PciInfoList *qmp_query_pci(Error **errp) return head; } -static const char * const pci_nic_models[] = { - "ne2k_pci", - "i82551", - "i82557b", - "i82559er", - "rtl8139", - "e1000", - "pcnet", - "virtio", - "sungem", - NULL -}; - -static const char * const pci_nic_names[] = { - "ne2k_pci", - "i82551", - "i82557b", - "i82559er", - "rtl8139", - "e1000", - "pcnet", - "virtio-net-pci", - "sungem", - NULL -}; - /* Initialize a PCI NIC. */ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus, const char *default_model, const char *default_devaddr) { const char *devaddr = nd->devaddr ? nd->devaddr : default_devaddr; + GSList *list; + GPtrArray *pci_nic_models; PCIBus *bus; PCIDevice *pci_dev; DeviceState *dev; int devfn; int i; - if (qemu_show_nic_models(nd->model, pci_nic_models)) { + if (!strcmp(nd->model, "virtio")) { + g_free(nd->model); + nd->model = g_strdup("virtio-net-pci"); + } + + list = object_class_get_list_sorted(TYPE_PCI_DEVICE, false); + pci_nic_models = g_ptr_array_new_with_free_func(g_free); + 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) ; + g_ptr_array_add(pci_nic_models, (gpointer)name); + } + next = list->next; + g_slist_free_1(list); + list = next; + } + g_ptr_array_add(pci_nic_models, NULL); + + if (qemu_show_nic_models(nd->model, (const char **)pci_nic_models->pdata)) { exit(0); } - i = qemu_find_nic_model(nd, pci_nic_models, default_model); + i = qemu_find_nic_model(nd, (const char **)pci_nic_models->pdata, + default_model); if (i < 0) { exit(1); } @@ -1865,15 +1864,15 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus, bus = pci_get_bus_devfn(&devfn, rootbus, devaddr); if (!bus) { error_report("Invalid PCI device address %s for device %s", - devaddr, pci_nic_names[i]); + devaddr, nd->model); exit(1); } - pci_dev = pci_create(bus, devfn, pci_nic_names[i]); + pci_dev = pci_create(bus, devfn, nd->model); dev = &pci_dev->qdev; qdev_set_nic_properties(dev, nd); qdev_init_nofail(dev); - + g_ptr_array_free(pci_nic_models, true); return pci_dev; }
Remove the hard-coded list of PCI NIC names; instead, fill an array using all PCI devices listed under DEVICE_CATEGORY_NETWORK. Keep the old shortcut "virtio" for virtio-net-pci. Suggested-by: Thomas Huth <thuth@redhat.com> Cc: Jason Wang <jasowang@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/pci/pci.c | 61 ++++++++++++++++++++++++++++++------------------------------ 1 file changed, 30 insertions(+), 31 deletions(-)