[Qemu-devel] [PATCH] replace duplicated code.

[Qemu-devel] [PATCH] replace duplicated code.

[Glauber Costa]

pci_register_simple() does exactly the same of great part of the code
in pci nic initialization. Replace it.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pci.c  |   12 ++++--------
 hw/qdev.c |    1 +
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index baffadd..0169b78 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -818,20 +818,16 @@ static const char * const pci_nic_names[] = {
 PCIDevice *pci_nic_init(PCIBus *bus, NICInfo *nd, int devfn,
                   const char *default_model)
 {
-    DeviceState *dev;
     int i;
 
     qemu_check_nic_model_list(nd, pci_nic_models, default_model);
 
     for (i = 0; pci_nic_models[i]; i++) {
         if (strcmp(nd->model, pci_nic_models[i]) == 0) {
-            dev = qdev_create(&bus->qbus, pci_nic_names[i]);
-            qdev_set_prop_int(dev, "devfn", devfn);
-            qdev_set_prop_ptr(dev, "name", (void *)pci_nic_names[i]);
-            qdev_set_netdev(dev, nd);
-            qdev_init(dev);
-            nd->private = dev;
-            return (PCIDevice *)dev;
+            PCIDevice *d;
+            d = pci_create_simple(bus, devfn, pci_nic_names[i]);
+            qdev_set_netdev((DeviceState *)d, nd);
+            return d;
         }
     }
 
diff --git a/hw/qdev.c b/hw/qdev.c
index 5175fe1..c7446d5 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -164,6 +164,7 @@ void qdev_set_netdev(DeviceState *dev, NICInfo *nd)
 {
     assert(!dev->nd);
     dev->nd = nd;
+    nd->private = dev;
 }
 
 
-- 
1.6.2.2

Re: [Qemu-devel] [PATCH] replace duplicated code.

[Mark McLoughlin]

On Wed, 2009-06-17 at 18:52 -0400, Glauber Costa wrote:
> pci_register_simple() does exactly the same of great part of the code
> in pci nic initialization. Replace it.
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/pci.c  |   12 ++++--------
>  hw/qdev.c |    1 +
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index baffadd..0169b78 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -818,20 +818,16 @@ static const char * const pci_nic_names[] = {
>  PCIDevice *pci_nic_init(PCIBus *bus, NICInfo *nd, int devfn,
>                    const char *default_model)
>  {
> -    DeviceState *dev;
>      int i;
>  
>      qemu_check_nic_model_list(nd, pci_nic_models, default_model);
>  
>      for (i = 0; pci_nic_models[i]; i++) {
>          if (strcmp(nd->model, pci_nic_models[i]) == 0) {
> -            dev = qdev_create(&bus->qbus, pci_nic_names[i]);
> -            qdev_set_prop_int(dev, "devfn", devfn);
> -            qdev_set_prop_ptr(dev, "name", (void *)pci_nic_names[i]);

So, this patch depends on your "allow for name property" patch - resend
as a series, maybe with this one first?

> -            qdev_set_netdev(dev, nd);
> -            qdev_init(dev);
> -            nd->private = dev;
> -            return (PCIDevice *)dev;
> +            PCIDevice *d;
> +            d = pci_create_simple(bus, devfn, pci_nic_names[i]);
> +            qdev_set_netdev((DeviceState *)d, nd);

I'd prefer &d->qdev here for type safety.

Cheers,
Mark.

Re: [Qemu-devel] [PATCH] replace duplicated code.

[Gerd Hoffmann]

> -            dev = qdev_create(&bus->qbus, pci_nic_names[i]);
> -            qdev_set_prop_int(dev, "devfn", devfn);
> -            qdev_set_prop_ptr(dev, "name", (void *)pci_nic_names[i]);
> -            qdev_set_netdev(dev, nd);
> -            qdev_init(dev);

> +            d = pci_create_simple(bus, devfn, pci_nic_names[i]);
> +            qdev_set_netdev((DeviceState *)d, nd);

You have changed the ordering of the qdev_set_netdev and qdev_init 
calls.  I don't think this is correct, the init callback will not see 
the nicinfo then.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] replace duplicated code.

[Glauber Costa]

On Thu, Jun 18, 2009 at 11:48:28AM +0200, Gerd Hoffmann wrote:
>> -            dev = qdev_create(&bus->qbus, pci_nic_names[i]);
>> -            qdev_set_prop_int(dev, "devfn", devfn);
>> -            qdev_set_prop_ptr(dev, "name", (void *)pci_nic_names[i]);
>> -            qdev_set_netdev(dev, nd);
>> -            qdev_init(dev);
>
>> +            d = pci_create_simple(bus, devfn, pci_nic_names[i]);
>> +            qdev_set_netdev((DeviceState *)d, nd);
>
> You have changed the ordering of the qdev_set_netdev and qdev_init  
> calls.  I don't think this is correct, the init callback will not see  
> the nicinfo then.
I see.

How would you feel about changing the nd field of qdev for a void * one,
call it private, or whatever?

then we could have a generic qdev_set_private(dev, p), that does it for
everybody. I must say I kinda dislike that specific dependency on nd for
net devices. Ideally, all devices should be treated as the same.

Re: [Qemu-devel] [PATCH] replace duplicated code.

[Gerd Hoffmann]

On 06/18/09 15:59, Glauber Costa wrote:
>> You have changed the ordering of the qdev_set_netdev and qdev_init
>> calls.  I don't think this is correct, the init callback will not see
>> the nicinfo then.
> I see.
>
> How would you feel about changing the nd field of qdev for a void * one,
> call it private, or whatever?
>
> then we could have a generic qdev_set_private(dev, p), that does it for
> everybody. I must say I kinda dislike that specific dependency on nd for
> net devices. Ideally, all devices should be treated as the same.

What problem you are trying to solve?
Unless there is something urgent to fix I'd leave it as is for now.

We need some sane way to link guest config (hopefully done by config 
file soon) to host config.  Link nic to vlan in that case, but disks and 
chardevs share the same issue.  I consider DeviceState->nd being 
temporary and expect will go away once we figured that out.  It is a 
bigger undertaking though ...

cheers,
   Gerd