Patchwork [13/17] PCI: Convert pci_device_hot_add() to QObject

login
register
mail settings
Submitter Luiz Capitulino
Date Nov. 17, 2009, 8:32 p.m.
Message ID <1258489944-12159-14-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/38691/
State New
Headers show

Comments

Luiz Capitulino - Nov. 17, 2009, 8:32 p.m.
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(-)
Markus Armbruster - Nov. 20, 2009, 2:21 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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)
> +{
> +    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"));

Despite its name, qdict_get_int() doesn't return int, but int64_t (trap
for the unwary, in my opinion), so these casts are actually needed.
The're safe, because the values put into the dictionary never overflow
the cast.

Would be nice if we could avoid this non-local argument and make the
code more obviously correct, but it's probably not worth the bother.

> +
> +}
> +
> +/**
> + * 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));
> +        assert(*ret_data != NULL);
>      } else
>          monitor_printf(mon, "failed to add %s\n", opts);
>  }
[...]
Michael S. Tsirkin - Nov. 23, 2009, 9:44 a.m.
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?

> +{
> +    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.

> +        assert(*ret_data != NULL);
>      } 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

>          .help       = "hot-add PCI device",
> -        .mhandler.cmd = pci_device_hot_add,
> +        .user_print = pci_device_hot_add_print,
> +        .mhandler.cmd_new = pci_device_hot_add,
>      },
>  #endif
>  
> diff --git a/sysemu.h b/sysemu.h
> index b1887ef..4d685a0 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -201,7 +201,8 @@ extern DriveInfo *drive_init(QemuOpts *arg, void *machine, int *fatal_error);
>  DriveInfo *add_init_drive(const char *opts);
>  
>  /* pci-hotplug */
> -void pci_device_hot_add(Monitor *mon, const QDict *qdict);
> +void pci_device_hot_add_print(Monitor *mon, const QObject *data);
> +void pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  void drive_hot_add(Monitor *mon, const QDict *qdict);
>  void pci_device_hot_remove(Monitor *mon, const char *pci_addr);
>  void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict,
> -- 
> 1.6.5.2.180.gc5b3e
> 
>
Luiz Capitulino - Nov. 23, 2009, 1:30 p.m.
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.

> > +        assert(*ret_data != NULL);
> >      } 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.
Michael S. Tsirkin - Nov. 30, 2009, 10:31 a.m.
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.
>

Patch

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)
+{
+    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));
+        assert(*ret_data != NULL);
     } 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]...",
         .help       = "hot-add PCI device",
-        .mhandler.cmd = pci_device_hot_add,
+        .user_print = pci_device_hot_add_print,
+        .mhandler.cmd_new = pci_device_hot_add,
     },
 #endif
 
diff --git a/sysemu.h b/sysemu.h
index b1887ef..4d685a0 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -201,7 +201,8 @@  extern DriveInfo *drive_init(QemuOpts *arg, void *machine, int *fatal_error);
 DriveInfo *add_init_drive(const char *opts);
 
 /* pci-hotplug */
-void pci_device_hot_add(Monitor *mon, const QDict *qdict);
+void pci_device_hot_add_print(Monitor *mon, const QObject *data);
+void pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
 void drive_hot_add(Monitor *mon, const QDict *qdict);
 void pci_device_hot_remove(Monitor *mon, const char *pci_addr);
 void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict,