[Qemu-devel] Re: [PATCH 13/17] PCI: Convert pci_device_hot_add() to QObject

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Nov 23, 2009 at 11:30:44AM -0200, Luiz Capitulino wrote:
> On Mon, 23 Nov 2009 11:44:57 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Nov 17, 2009 at 06:32:20PM -0200, Luiz Capitulino wrote:
> > > Return a QDict with information about the just added device.
> > > 
> > > This commit should not change user output.
> > > 
> > > Please, note that this patch does not do error handling
> > > conversion. In error conditions the handler still calls
> > > monitor_printf().
> > > 
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > ---
> > >  hw/pci-hotplug.c |   37 +++++++++++++++++++++++++++++++++----
> > >  qemu-monitor.hx  |    3 ++-
> > >  sysemu.h         |    3 ++-
> > >  3 files changed, 37 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
> > > index a254498..93802a2 100644
> > > --- a/hw/pci-hotplug.c
> > > +++ b/hw/pci-hotplug.c
> > > @@ -33,6 +33,7 @@
> > >  #include "scsi.h"
> > >  #include "virtio-blk.h"
> > >  #include "qemu-config.h"
> > > +#include "qemu-objects.h"
> > >  
> > >  #if defined(TARGET_I386)
> > >  static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
> > > @@ -212,7 +213,36 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
> > >      return dev;
> > >  }
> > >  
> > > -void pci_device_hot_add(Monitor *mon, const QDict *qdict)
> > > +void pci_device_hot_add_print(Monitor *mon, const QObject *data)
> > 
> > it only prints - why the strange name?
> 
>  I'm adding the _print suffix to the functions which print
> handler data, that's, handle_name + _print.
> 
>  It may have strange results, indeed, but sometimes it's difficult
> to come with good names as I'm converting several handlers.
> 
>  Also, I'd like to have standard names and haven't decided for
> one yet.
> 
> > > +{
> > > +    QDict *qdict;
> > > +
> > > +    assert(qobject_type(data) == QTYPE_QDICT);
> > > +    qdict = qobject_to_qdict(data);
> > > +
> > > +    monitor_printf(mon, "OK domain %d, bus %d, slot %d, function %d\n",
> > > +                   (int) qdict_get_int(qdict, "domain"),
> > > +                   (int) qdict_get_int(qdict, "bus"),
> > > +                   (int) qdict_get_int(qdict, "slot"),
> > > +                   (int) qdict_get_int(qdict, "function"));
> > > +
> > > +}
> > > +
> > > +/**
> > > + * pci_device_hot_add(): Hot add PCI device
> > > + *
> > > + * Return a QDict with the following device information:
> > > + *
> > > + * - "domain": domain number
> > > + * - "bus": bus number
> > > + * - "slot": slot number
> > > + * - "function": function number
> > > + *
> > > + * Example:
> > > + *
> > > + * { "domain": 0, "bus": 0, "slot": 5, "function": 0 }
> > > + */
> > > +void pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > >  {
> > >      PCIDevice *dev = NULL;
> > >      const char *pci_addr = qdict_get_str(qdict, "pci_addr");
> > > @@ -239,9 +269,8 @@ void pci_device_hot_add(Monitor *mon, const QDict *qdict)
> > >          monitor_printf(mon, "invalid type: %s\n", type);
> > >  
> > >      if (dev) {
> > > -        monitor_printf(mon, "OK domain %d, bus %d, slot %d, function %d\n",
> > > -                       0, pci_bus_num(dev->bus), PCI_SLOT(dev->devfn),
> > > -                       PCI_FUNC(dev->devfn));
> > > +        *ret_data = qobject_from_jsonf("{ 'domain': 0, 'bus': %d, 'slot': %d, 'function': %d }", pci_bus_num(dev->bus), PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> > 
> > this line is waay too long.
> 
>  I can brake it, although I don't think the result is going to be
> prettier.

Here's one that looks decent to me.

        *ret_data = qobject_from_jsonf("{ 'domain': 0,"
                                        " 'bus': %d,"
                                        " 'slot': %d,"
			                " 'function': %d }",
				       pci_bus_num(dev->bus),
				       PCI_SLOT(dev->devfn),
				       PCI_FUNC(dev->devfn));


> > > +        assert(*ret_data != NULL);

BTW, pls make this assert(*ret_data);

> > >      } else
> > >          monitor_printf(mon, "failed to add %s\n", opts);
> > >  }
> > > diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> > > index 62e395b..b50a2da 100644
> > > --- a/qemu-monitor.hx
> > > +++ b/qemu-monitor.hx
> > > @@ -809,7 +809,8 @@ ETEXI
> > >          .args_type  = "pci_addr:s,type:s,opts:s?",
> > >          .params     = "auto|[[<domain>:]<bus>:]<slot> nic|storage [[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]...",
> > 
> > 
> > and this
> 
>  This line is not part of the series.
>