[Qemu-devel] [PATCH 0/2]: convert device_del to the qapi

[Qemu-devel] [PATCH 0/2]: convert device_del to the qapi

[Luiz Capitulino]

Please, check individual patches for details.

 hmp-commands.hx      |    3 +--
 hmp.c                |    9 +++++++++
 hmp.h                |    1 +
 hw/pci-hotplug.c     |   15 ++++++++++++---
 hw/qdev-monitor.c    |   18 +++++++++++++-----
 hw/qdev.c            |    5 +++--
 hw/qdev.h            |    3 ++-
 hw/usb/dev-storage.c |    2 +-
 hw/xen_platform.c    |    4 ++--
 qapi-schema.json     |   20 ++++++++++++++++++++
 qmp-commands.hx      |    5 +----
 11 files changed, 65 insertions(+), 20 deletions(-)

[Qemu-devel] [PATCH 1/2] qdev: qdev_unplug(): Use error_set()

[Luiz Capitulino]

It currently uses qerror_report(), but next commits will convert
the drive_del command to the QAPI and this requires using
error_set().

One particularity of qerror_report() is that it knows when it's
running on monitor context or command-line context and prints the
error message accordingly. error_set() doesn't do this, so we
have to be careful not to drop error messages.

qdev_unplug() has three kinds of usages:

 1. It's called when hot adding a device fails, to undo anything
    that has been done before hitting the error

 2. It's called by function monitor functions like device_del(),
    to unplug a device

 3. It's used by xen_platform.c in a way that doesn't _seem_ to
    be in monitor context

Only item 2 can print an error message to the user, this commit
maintains that.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/pci-hotplug.c     |   15 ++++++++++++---
 hw/qdev-monitor.c    |   11 ++++++++++-
 hw/qdev.c            |    5 +++--
 hw/qdev.h            |    3 ++-
 hw/usb/dev-storage.c |    2 +-
 hw/xen_platform.c    |    4 ++--
 6 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 5c6307f..a060917 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -32,6 +32,7 @@
 #include "virtio-blk.h"
 #include "qemu-config.h"
 #include "blockdev.h"
+#include "error.h"
 
 #if defined(TARGET_I386)
 static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
@@ -191,7 +192,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
             dev = NULL;
         if (dev && dinfo) {
             if (scsi_hot_add(mon, &dev->qdev, dinfo, 0) != 0) {
-                qdev_unplug(&dev->qdev);
+                qdev_unplug(&dev->qdev, NULL);
                 dev = NULL;
             }
         }
@@ -256,8 +257,9 @@ void pci_device_hot_add(Monitor *mon, const QDict *qdict)
 static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
 {
     PCIDevice *d;
-    int dom, bus;
+    int dom, bus, ret;
     unsigned slot;
+    Error *errp = NULL;
 
     if (pci_read_devaddr(mon, pci_addr, &dom, &bus, &slot)) {
         return -1;
@@ -268,7 +270,14 @@ static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
         monitor_printf(mon, "slot %d empty\n", slot);
         return -1;
     }
-    return qdev_unplug(&d->qdev);
+
+    ret = qdev_unplug(&d->qdev, &errp);
+    if (error_is_set(&errp)) {
+        monitor_printf(mon, "%s\n", error_get_pretty(errp));
+        error_free(errp);
+    }
+
+    return ret;
 }
 
 void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict)
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index a310cc7..58fa943 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -578,13 +578,22 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *id = qdict_get_str(qdict, "id");
     DeviceState *dev;
+    Error *local_err = NULL;
+    int ret;
 
     dev = qdev_find_recursive(sysbus_get_default(), id);
     if (NULL == dev) {
         qerror_report(QERR_DEVICE_NOT_FOUND, id);
         return -1;
     }
-    return qdev_unplug(dev);
+
+    ret = qdev_unplug(dev, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+
+    return ret;
 }
 
 void qdev_machine_init(void)
diff --git a/hw/qdev.c b/hw/qdev.c
index ee21d90..cf2a9a9 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -28,6 +28,7 @@
 #include "net.h"
 #include "qdev.h"
 #include "sysemu.h"
+#include "error.h"
 
 int qdev_hotplug = 0;
 static bool qdev_hot_added = false;
@@ -172,12 +173,12 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
     dev->alias_required_for_version = required_for_version;
 }
 
-int qdev_unplug(DeviceState *dev)
+int qdev_unplug(DeviceState *dev, Error **errp)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
     if (!dev->parent_bus->allow_hotplug) {
-        qerror_report(QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
+        error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
         return -1;
     }
     assert(dc->unplug != NULL);
diff --git a/hw/qdev.h b/hw/qdev.h
index 9cc3f98..22cf490 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -7,6 +7,7 @@
 #include "qemu-option.h"
 #include "qapi/qapi-visit-core.h"
 #include "qemu/object.h"
+#include "error.h"
 
 typedef struct Property Property;
 
@@ -148,7 +149,7 @@ int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
 void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
-int qdev_unplug(DeviceState *dev);
+int qdev_unplug(DeviceState *dev, Error **errp);
 void qdev_free(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
 void qdev_machine_creation_done(void);
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index bdbe7bd..d865a5e 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -499,7 +499,7 @@ static void usb_msd_password_cb(void *opaque, int err)
         err = usb_device_attach(&s->dev);
 
     if (err)
-        qdev_unplug(&s->dev.qdev);
+        qdev_unplug(&s->dev.qdev, NULL);
 }
 
 static const struct SCSIBusInfo usb_msd_scsi_info = {
diff --git a/hw/xen_platform.c b/hw/xen_platform.c
index 5a7c4cc..a9c52a6 100644
--- a/hw/xen_platform.c
+++ b/hw/xen_platform.c
@@ -87,7 +87,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d)
 {
     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
             PCI_CLASS_NETWORK_ETHERNET) {
-        qdev_unplug(&(d->qdev));
+        qdev_unplug(&(d->qdev), NULL);
     }
 }
 
@@ -100,7 +100,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d)
 {
     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
             PCI_CLASS_STORAGE_IDE) {
-        qdev_unplug(&(d->qdev));
+        qdev_unplug(&(d->qdev), NULL);
     }
 }
 
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 2/2] qapi: convert device_del

[Luiz Capitulino]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx   |    3 +--
 hmp.c             |    9 +++++++++
 hmp.h             |    1 +
 hw/qdev-monitor.c |   15 +++++++--------
 qapi-schema.json  |   20 ++++++++++++++++++++
 qmp-commands.hx   |    5 +----
 6 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index bd35a3e..a6f5a84 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -622,8 +622,7 @@ ETEXI
         .args_type  = "id:s",
         .params     = "device",
         .help       = "remove device",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_device_del,
+        .mhandler.cmd = hmp_device_del,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 9cf2d13..f3e5163 100644
--- a/hmp.c
+++ b/hmp.c
@@ -934,3 +934,12 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
         qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
     }
 }
+
+void hmp_device_del(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    Error *err = NULL;
+
+    qmp_device_del(id, &err);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 8807853..443b812 100644
--- a/hmp.h
+++ b/hmp.h
@@ -60,5 +60,6 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict);
 void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
 void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate(Monitor *mon, const QDict *qdict);
+void hmp_device_del(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index 58fa943..761ba90 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -19,6 +19,7 @@
 
 #include "qdev.h"
 #include "monitor.h"
+#include "qmp-commands.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -574,26 +575,24 @@ int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return 0;
 }
 
-int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_device_del(const char *id, Error **errp)
 {
-    const char *id = qdict_get_str(qdict, "id");
     DeviceState *dev;
     Error *local_err = NULL;
     int ret;
 
     dev = qdev_find_recursive(sysbus_get_default(), id);
     if (NULL == dev) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, id);
-        return -1;
+        error_set(errp, QERR_DEVICE_NOT_FOUND, id);
+        return;
     }
 
     ret = qdev_unplug(dev, &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
+    } else if (ret) {
+        error_set(errp, QERR_UNDEFINED_ERROR);
     }
-
-    return ret;
 }
 
 void qdev_machine_init(void)
diff --git a/qapi-schema.json b/qapi-schema.json
index 0d11d6e..ace55f3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1701,3 +1701,23 @@
 # Since: 1.1
 ##
 { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
+
+##
+# @device_del:
+#
+# Remove a device from a guest
+#
+# @id: the name of the device
+#
+# Returns: Nothing on success
+#          If @id is not a valid device, DeviceNotFound
+#          If the device does not support unplug, BusNoHotplug
+#
+# Notes: When this command completes, the device may not be removed from the
+#        guest.  Hot removal is an operation that requires guest cooperation.
+#        This command merely requests that the guest begin the hot removal
+#        process.
+#
+# Since: 0.14.0
+##
+{ 'command': 'device_del', 'data': {'id': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c626ba8..c878b54 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -314,10 +314,7 @@ EQMP
     {
         .name       = "device_del",
         .args_type  = "id:s",
-        .params     = "device",
-        .help       = "remove device",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_device_del,
+        .mhandler.cmd_new = qmp_marshal_input_device_del,
     },
 
 SQMP
-- 
1.7.9.2.384.g4a92a

Re: [Qemu-devel] [PATCH 1/2] qdev: qdev_unplug(): Use error_set()

[Stefan Hajnoczi]

On Wed, Mar 28, 2012 at 05:50:53PM -0300, Luiz Capitulino wrote:
> @@ -268,7 +270,14 @@ static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
>          monitor_printf(mon, "slot %d empty\n", slot);
>          return -1;
>      }
> -    return qdev_unplug(&d->qdev);
> +
> +    ret = qdev_unplug(&d->qdev, &errp);
> +    if (error_is_set(&errp)) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(errp));
> +        error_free(errp);
> +    }

Minor thing if you respin: this if statement could be replaced with hmp_handle_error(mon, &errp).

Stefan

Re: [Qemu-devel] [PATCH 2/2] qapi: convert device_del

[Stefan Hajnoczi]

On Wed, Mar 28, 2012 at 05:50:54PM -0300, Luiz Capitulino wrote:
>      ret = qdev_unplug(dev, &local_err);
>      if (error_is_set(&local_err)) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> +        error_propagate(errp, local_err);
> +    } else if (ret) {
> +        error_set(errp, QERR_UNDEFINED_ERROR);

Can we make qdev_unplug() void?  I can find no case in QEMU where we
return != 0 without setting error.  If we fix the function prototype
this invalid state can be eliminated forever.

(Other functions that take Error **errp are usually void.)

> diff --git a/qapi-schema.json b/qapi-schema.json
> index 0d11d6e..ace55f3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1701,3 +1701,23 @@
>  # Since: 1.1
>  ##
>  { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
> +
> +##
> +# @device_del:
> +#
> +# Remove a device from a guest
> +#
> +# @id: the name of the device
> +#
> +# Returns: Nothing on success
> +#          If @id is not a valid device, DeviceNotFound
> +#          If the device does not support unplug, BusNoHotplug
> +#
> +# Notes: When this command completes, the device may not be removed from the
> +#        guest.  Hot removal is an operation that requires guest cooperation.
> +#        This command merely requests that the guest begin the hot removal
> +#        process.

I have not peeked at the implementation in QEMU or libvirt, but is there
a QMP event for actual removal or would the user need to poll?  This bit
of information would be useful in the documentation.

Stefan

Re: [Qemu-devel] [PATCH 1/2] qdev: qdev_unplug(): Use error_set()

[Luiz Capitulino]

On Thu, 29 Mar 2012 08:00:15 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Mar 28, 2012 at 05:50:53PM -0300, Luiz Capitulino wrote:
> > @@ -268,7 +270,14 @@ static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
> >          monitor_printf(mon, "slot %d empty\n", slot);
> >          return -1;
> >      }
> > -    return qdev_unplug(&d->qdev);
> > +
> > +    ret = qdev_unplug(&d->qdev, &errp);
> > +    if (error_is_set(&errp)) {
> > +        monitor_printf(mon, "%s\n", error_get_pretty(errp));
> > +        error_free(errp);
> > +    }
> 
> Minor thing if you respin: this if statement could be replaced with hmp_handle_error(mon, &errp).

I'm not sure I'd like to see hmp_handle_error() spread over the tree. It uses
the monitor object and I've added it just because having the same code
duplicated among HMP functions bothered me... I think it's better to
restrict it to hmp.c.

Re: [Qemu-devel] [PATCH 2/2] qapi: convert device_del

[Luiz Capitulino]

On Thu, 29 Mar 2012 08:08:51 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Mar 28, 2012 at 05:50:54PM -0300, Luiz Capitulino wrote:
> >      ret = qdev_unplug(dev, &local_err);
> >      if (error_is_set(&local_err)) {
> > -        qerror_report_err(local_err);
> > -        error_free(local_err);
> > +        error_propagate(errp, local_err);
> > +    } else if (ret) {
> > +        error_set(errp, QERR_UNDEFINED_ERROR);
> 
> Can we make qdev_unplug() void?  I can find no case in QEMU where we
> return != 0 without setting error.  If we fix the function prototype
> this invalid state can be eliminated forever.

Good point, I'll change it.

> 
> (Other functions that take Error **errp are usually void.)
> 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 0d11d6e..ace55f3 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1701,3 +1701,23 @@
> >  # Since: 1.1
> >  ##
> >  { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
> > +
> > +##
> > +# @device_del:
> > +#
> > +# Remove a device from a guest
> > +#
> > +# @id: the name of the device
> > +#
> > +# Returns: Nothing on success
> > +#          If @id is not a valid device, DeviceNotFound
> > +#          If the device does not support unplug, BusNoHotplug
> > +#
> > +# Notes: When this command completes, the device may not be removed from the
> > +#        guest.  Hot removal is an operation that requires guest cooperation.
> > +#        This command merely requests that the guest begin the hot removal
> > +#        process.
> 
> I have not peeked at the implementation in QEMU or libvirt, but is there
> a QMP event for actual removal or would the user need to poll?  This bit
> of information would be useful in the documentation.

There's no event, I'll document it. Is there any preferred method for
polling? query-pci?

Re: [Qemu-devel] [PATCH 2/2] qapi: convert device_del

[Stefan Hajnoczi]

On Thu, Mar 29, 2012 at 2:17 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Thu, 29 Mar 2012 08:08:51 +0100
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
>> On Wed, Mar 28, 2012 at 05:50:54PM -0300, Luiz Capitulino wrote:
>> >      ret = qdev_unplug(dev, &local_err);
>> >      if (error_is_set(&local_err)) {
>> > -        qerror_report_err(local_err);
>> > -        error_free(local_err);
>> > +        error_propagate(errp, local_err);
>> > +    } else if (ret) {
>> > +        error_set(errp, QERR_UNDEFINED_ERROR);
>>
>> Can we make qdev_unplug() void?  I can find no case in QEMU where we
>> return != 0 without setting error.  If we fix the function prototype
>> this invalid state can be eliminated forever.
>
> Good point, I'll change it.
>
>>
>> (Other functions that take Error **errp are usually void.)
>>
>> > diff --git a/qapi-schema.json b/qapi-schema.json
>> > index 0d11d6e..ace55f3 100644
>> > --- a/qapi-schema.json
>> > +++ b/qapi-schema.json
>> > @@ -1701,3 +1701,23 @@
>> >  # Since: 1.1
>> >  ##
>> >  { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
>> > +
>> > +##
>> > +# @device_del:
>> > +#
>> > +# Remove a device from a guest
>> > +#
>> > +# @id: the name of the device
>> > +#
>> > +# Returns: Nothing on success
>> > +#          If @id is not a valid device, DeviceNotFound
>> > +#          If the device does not support unplug, BusNoHotplug
>> > +#
>> > +# Notes: When this command completes, the device may not be removed from the
>> > +#        guest.  Hot removal is an operation that requires guest cooperation.
>> > +#        This command merely requests that the guest begin the hot removal
>> > +#        process.
>>
>> I have not peeked at the implementation in QEMU or libvirt, but is there
>> a QMP event for actual removal or would the user need to poll?  This bit
>> of information would be useful in the documentation.
>
> There's no event, I'll document it. Is there any preferred method for
> polling? query-pci?

I took a quick peek at libvirt and am none the wiser.  If we don't
know what the recommended approach is then let's leave it.

Stefan

Re: [Qemu-devel] [PATCH 1/2] qdev: qdev_unplug(): Use error_set()

[Stefan Hajnoczi]

On Thu, Mar 29, 2012 at 2:15 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Thu, 29 Mar 2012 08:00:15 +0100
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
>> On Wed, Mar 28, 2012 at 05:50:53PM -0300, Luiz Capitulino wrote:
>> > @@ -268,7 +270,14 @@ static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
>> >          monitor_printf(mon, "slot %d empty\n", slot);
>> >          return -1;
>> >      }
>> > -    return qdev_unplug(&d->qdev);
>> > +
>> > +    ret = qdev_unplug(&d->qdev, &errp);
>> > +    if (error_is_set(&errp)) {
>> > +        monitor_printf(mon, "%s\n", error_get_pretty(errp));
>> > +        error_free(errp);
>> > +    }
>>
>> Minor thing if you respin: this if statement could be replaced with hmp_handle_error(mon, &errp).
>
> I'm not sure I'd like to see hmp_handle_error() spread over the tree. It uses
> the monitor object and I've added it just because having the same code
> duplicated among HMP functions bothered me... I think it's better to
> restrict it to hmp.c.

Okay.  I mentioned it because I noticed that there are several
different ways to do essentially the same thing.

Stefan

Re: [Qemu-devel] [PATCH 2/2] qapi: convert device_del

[Luiz Capitulino]

On Thu, 29 Mar 2012 11:55:31 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 03/29/2012 10:56 AM, Luiz Capitulino wrote:
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hmp-commands.hx   |    3 +--
> >  hmp.c             |    9 +++++++++
> >  hmp.h             |    1 +
> >  hw/qdev-monitor.c |   18 +++++-------------
> >  qapi-schema.json  |   20 ++++++++++++++++++++
> >  qmp-commands.hx   |    5 +----
> >  6 files changed, 37 insertions(+), 19 deletions(-)
> 
> > +# @device_del:
> > +#
> > +# Remove a device from a guest
> > +#
> > +# @id: the name of the device
> > +#
> > +# Returns: Nothing on success
> > +#          If @id is not a valid device, DeviceNotFound
> > +#          If the device does not support unplug, BusNoHotplug
> > +#
> > +# Notes: When this command completes, the device may not be removed from the
> > +#        guest.  Hot removal is an operation that requires guest cooperation.
> > +#        This command merely requests that the guest begin the hot removal
> > +#        process.
> 
> Nothing against your patch itself, but is there any way we could enhance
> things in future patches to actually notify the management app when the
> guest has cooperated and the devices is actually removed?  A new event
> would be helpful, as well as a way to detect whether the new event
> exists and should be waited for.

To allow to query for the existence of the event, we need to add the
'event' type to the qapi and allow clients to query our schema. Maybe we
could try it for 1.2.

The 'device removed' event could be added now, but I think it's better to
wait for the QOM refactoring because there might be better ways to do it.

Re: [Qemu-devel] [PATCH 2/2] qapi: convert device_del

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

On 03/29/2012 10:56 AM, Luiz Capitulino wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hmp-commands.hx   |    3 +--
>  hmp.c             |    9 +++++++++
>  hmp.h             |    1 +
>  hw/qdev-monitor.c |   18 +++++-------------
>  qapi-schema.json  |   20 ++++++++++++++++++++
>  qmp-commands.hx   |    5 +----
>  6 files changed, 37 insertions(+), 19 deletions(-)

> +# @device_del:
> +#
> +# Remove a device from a guest
> +#
> +# @id: the name of the device
> +#
> +# Returns: Nothing on success
> +#          If @id is not a valid device, DeviceNotFound
> +#          If the device does not support unplug, BusNoHotplug
> +#
> +# Notes: When this command completes, the device may not be removed from the
> +#        guest.  Hot removal is an operation that requires guest cooperation.
> +#        This command merely requests that the guest begin the hot removal
> +#        process.

Nothing against your patch itself, but is there any way we could enhance
things in future patches to actually notify the management app when the
guest has cooperated and the devices is actually removed?  A new event
would be helpful, as well as a way to detect whether the new event
exists and should be waited for.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[Qemu-devel] [PATCH 2/2] qapi: convert device_del

[Luiz Capitulino]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx   |    3 +--
 hmp.c             |    9 +++++++++
 hmp.h             |    1 +
 hw/qdev-monitor.c |   18 +++++-------------
 qapi-schema.json  |   20 ++++++++++++++++++++
 qmp-commands.hx   |    5 +----
 6 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index bd35a3e..a6f5a84 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -622,8 +622,7 @@ ETEXI
         .args_type  = "id:s",
         .params     = "device",
         .help       = "remove device",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_device_del,
+        .mhandler.cmd = hmp_device_del,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 9cf2d13..f3e5163 100644
--- a/hmp.c
+++ b/hmp.c
@@ -934,3 +934,12 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
         qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
     }
 }
+
+void hmp_device_del(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    Error *err = NULL;
+
+    qmp_device_del(id, &err);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 8807853..443b812 100644
--- a/hmp.h
+++ b/hmp.h
@@ -60,5 +60,6 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict);
 void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
 void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate(Monitor *mon, const QDict *qdict);
+void hmp_device_del(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index e952238..7e3c925 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -19,6 +19,7 @@
 
 #include "qdev.h"
 #include "monitor.h"
+#include "qmp-commands.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -574,26 +575,17 @@ int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return 0;
 }
 
-int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_device_del(const char *id, Error **errp)
 {
-    const char *id = qdict_get_str(qdict, "id");
-    Error *local_err = NULL;
     DeviceState *dev;
 
     dev = qdev_find_recursive(sysbus_get_default(), id);
     if (NULL == dev) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, id);
-        return -1;
-    }
-
-    qdev_unplug(dev, &local_err);
-    if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
+        error_set(errp, QERR_DEVICE_NOT_FOUND, id);
+        return;
     }
 
-    return 0;
+    qdev_unplug(dev, errp);
 }
 
 void qdev_machine_init(void)
diff --git a/qapi-schema.json b/qapi-schema.json
index 0d11d6e..ace55f3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1701,3 +1701,23 @@
 # Since: 1.1
 ##
 { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
+
+##
+# @device_del:
+#
+# Remove a device from a guest
+#
+# @id: the name of the device
+#
+# Returns: Nothing on success
+#          If @id is not a valid device, DeviceNotFound
+#          If the device does not support unplug, BusNoHotplug
+#
+# Notes: When this command completes, the device may not be removed from the
+#        guest.  Hot removal is an operation that requires guest cooperation.
+#        This command merely requests that the guest begin the hot removal
+#        process.
+#
+# Since: 0.14.0
+##
+{ 'command': 'device_del', 'data': {'id': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c626ba8..c878b54 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -314,10 +314,7 @@ EQMP
     {
         .name       = "device_del",
         .args_type  = "id:s",
-        .params     = "device",
-        .help       = "remove device",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_device_del,
+        .mhandler.cmd_new = qmp_marshal_input_device_del,
     },
 
 SQMP
-- 
1.7.9.2.384.g4a92a