[Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Marcel Apfelbaum]

Qemu is expected to quit if the same boot index value is used by two devices.
However, hot-plugging a device with a bootindex value already used should
fail with a friendly message rather than quitting a running VM.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 qdev-monitor.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 410cdcb..654d086 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -24,6 +24,7 @@
 #include "qmp-commands.h"
 #include "sysemu/arch_init.h"
 #include "qemu/config-file.h"
+#include "sysemu/sysemu.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -442,6 +443,31 @@ static BusState *qbus_find(const char *path)
     }
 }
 
+#define OBJ_PROP_BOOTINDEX "bootindex"
+
+static bool bootindex_colision(Object *obj, QemuOpts *opts)
+{
+    int32_t bootindex;
+
+    if (!object_property_find(obj, OBJ_PROP_BOOTINDEX, NULL)) {
+        return false;
+    }
+
+    /* avoid parsing by setting the property and then getting it typed */
+    object_property_parse(obj, qemu_opt_get(opts, OBJ_PROP_BOOTINDEX),
+                          OBJ_PROP_BOOTINDEX, NULL);
+    bootindex = (int32_t)object_property_get_int(obj, OBJ_PROP_BOOTINDEX,
+                                                 NULL);
+
+    if (bootindex >= 0) {
+        if (get_boot_device(bootindex)) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 DeviceState *qdev_device_add(QemuOpts *opts)
 {
     ObjectClass *obj;
@@ -502,6 +528,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     /* create device, set properties */
     qdev = DEVICE(object_new(driver));
 
+    if (qdev_hotplug && bootindex_colision(OBJECT(qdev), opts)) {
+        qerror_report(QERR_INVALID_PARAMETER_VALUE,
+                      "bootindex", "an unused boot index value");
+        qdev_free(qdev);
+        return NULL;
+    }
+
     if (bus) {
         qdev_set_parent_bus(qdev, bus);
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Marcel Apfelbaum]

On Wed, 2013-09-11 at 21:26 +0300, Marcel Apfelbaum wrote:
> Qemu is expected to quit if the same boot index value is used by two devices.
> However, hot-plugging a device with a bootindex value already used should
> fail with a friendly message rather than quitting a running VM.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  qdev-monitor.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 410cdcb..654d086 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -24,6 +24,7 @@
>  #include "qmp-commands.h"
>  #include "sysemu/arch_init.h"
>  #include "qemu/config-file.h"
> +#include "sysemu/sysemu.h"
>  
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
> @@ -442,6 +443,31 @@ static BusState *qbus_find(const char *path)
>      }
>  }
>  
> +#define OBJ_PROP_BOOTINDEX "bootindex"
A drawback of this patch is that is based on the assumption that
all devices supporting boot ordering will have a property with the
same name: bootindex.
A more robust approach would be to have some kind of interface
for such devices with a single "getter" method: bootindex()

Any thoughts?
Thanks,
Marcel 

> +
> +static bool bootindex_colision(Object *obj, QemuOpts *opts)
> +{
> +    int32_t bootindex;
> +
> +    if (!object_property_find(obj, OBJ_PROP_BOOTINDEX, NULL)) {
> +        return false;
> +    }
> +
> +    /* avoid parsing by setting the property and then getting it typed */
> +    object_property_parse(obj, qemu_opt_get(opts, OBJ_PROP_BOOTINDEX),
> +                          OBJ_PROP_BOOTINDEX, NULL);
> +    bootindex = (int32_t)object_property_get_int(obj, OBJ_PROP_BOOTINDEX,
> +                                                 NULL);
> +
> +    if (bootindex >= 0) {
> +        if (get_boot_device(bootindex)) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>  DeviceState *qdev_device_add(QemuOpts *opts)
>  {
>      ObjectClass *obj;
> @@ -502,6 +528,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      /* create device, set properties */
>      qdev = DEVICE(object_new(driver));
>  
> +    if (qdev_hotplug && bootindex_colision(OBJECT(qdev), opts)) {
> +        qerror_report(QERR_INVALID_PARAMETER_VALUE,
> +                      "bootindex", "an unused boot index value");
> +        qdev_free(qdev);
> +        return NULL;
> +    }
> +
>      if (bus) {
>          qdev_set_parent_bus(qdev, bus);
>      }

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Paolo Bonzini]

Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
> Qemu is expected to quit if the same boot index value is used by two devices.
> However, hot-plugging a device with a bootindex value already used should
> fail with a friendly message rather than quitting a running VM.

I think the problem is right where QEMU exits, i.e. in
add_boot_device_path.  This function should return an error instead, via
an Error ** argument.

Callers, typically a device's init or realize function, will either
print the error before returning an error code (e.g. -EBUSY for init) or
propagate the error up (for realize).

Returning/propagating failure will still cause QEMU to exit when the
duplicate bootindexes are found on the command line.

Paolo

> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  qdev-monitor.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 410cdcb..654d086 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -24,6 +24,7 @@
>  #include "qmp-commands.h"
>  #include "sysemu/arch_init.h"
>  #include "qemu/config-file.h"
> +#include "sysemu/sysemu.h"
>  
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
> @@ -442,6 +443,31 @@ static BusState *qbus_find(const char *path)
>      }
>  }
>  
> +#define OBJ_PROP_BOOTINDEX "bootindex"
> +
> +static bool bootindex_colision(Object *obj, QemuOpts *opts)
> +{
> +    int32_t bootindex;
> +
> +    if (!object_property_find(obj, OBJ_PROP_BOOTINDEX, NULL)) {
> +        return false;
> +    }
> +
> +    /* avoid parsing by setting the property and then getting it typed */
> +    object_property_parse(obj, qemu_opt_get(opts, OBJ_PROP_BOOTINDEX),
> +                          OBJ_PROP_BOOTINDEX, NULL);
> +    bootindex = (int32_t)object_property_get_int(obj, OBJ_PROP_BOOTINDEX,
> +                                                 NULL);
> +
> +    if (bootindex >= 0) {
> +        if (get_boot_device(bootindex)) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>  DeviceState *qdev_device_add(QemuOpts *opts)
>  {
>      ObjectClass *obj;
> @@ -502,6 +528,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      /* create device, set properties */
>      qdev = DEVICE(object_new(driver));
>  
> +    if (qdev_hotplug && bootindex_colision(OBJECT(qdev), opts)) {
> +        qerror_report(QERR_INVALID_PARAMETER_VALUE,
> +                      "bootindex", "an unused boot index value");
> +        qdev_free(qdev);
> +        return NULL;
> +    }
> +
>      if (bus) {
>          qdev_set_parent_bus(qdev, bus);
>      }
> 

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Marcel Apfelbaum]

On Thu, 2013-09-12 at 09:49 +0200, Paolo Bonzini wrote:
> Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
> > Qemu is expected to quit if the same boot index value is used by two devices.
> > However, hot-plugging a device with a bootindex value already used should
> > fail with a friendly message rather than quitting a running VM.
> 
> I think the problem is right where QEMU exits, i.e. in
> add_boot_device_path.  This function should return an error instead, via
> an Error ** argument.
> 
> Callers, typically a device's init or realize function, will either
> print the error before returning an error code (e.g. -EBUSY for init) or
> propagate the error up (for realize).
Thanks, I'll try this.
Marcel

> Returning/propagating failure will still cause QEMU to exit when the
> duplicate bootindexes are found on the command line.
> 
> Paolo
> 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  qdev-monitor.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 410cdcb..654d086 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -24,6 +24,7 @@
> >  #include "qmp-commands.h"
> >  #include "sysemu/arch_init.h"
> >  #include "qemu/config-file.h"
> > +#include "sysemu/sysemu.h"
> >  
> >  /*
> >   * Aliases were a bad idea from the start.  Let's keep them
> > @@ -442,6 +443,31 @@ static BusState *qbus_find(const char *path)
> >      }
> >  }
> >  
> > +#define OBJ_PROP_BOOTINDEX "bootindex"
> > +
> > +static bool bootindex_colision(Object *obj, QemuOpts *opts)
> > +{
> > +    int32_t bootindex;
> > +
> > +    if (!object_property_find(obj, OBJ_PROP_BOOTINDEX, NULL)) {
> > +        return false;
> > +    }
> > +
> > +    /* avoid parsing by setting the property and then getting it typed */
> > +    object_property_parse(obj, qemu_opt_get(opts, OBJ_PROP_BOOTINDEX),
> > +                          OBJ_PROP_BOOTINDEX, NULL);
> > +    bootindex = (int32_t)object_property_get_int(obj, OBJ_PROP_BOOTINDEX,
> > +                                                 NULL);
> > +
> > +    if (bootindex >= 0) {
> > +        if (get_boot_device(bootindex)) {
> > +            return true;
> > +        }
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  DeviceState *qdev_device_add(QemuOpts *opts)
> >  {
> >      ObjectClass *obj;
> > @@ -502,6 +528,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >      /* create device, set properties */
> >      qdev = DEVICE(object_new(driver));
> >  
> > +    if (qdev_hotplug && bootindex_colision(OBJECT(qdev), opts)) {
> > +        qerror_report(QERR_INVALID_PARAMETER_VALUE,
> > +                      "bootindex", "an unused boot index value");
> > +        qdev_free(qdev);
> > +        return NULL;
> > +    }
> > +
> >      if (bus) {
> >          qdev_set_parent_bus(qdev, bus);
> >      }
> > 
> 

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
>> Qemu is expected to quit if the same boot index value is used by two devices.
>> However, hot-plugging a device with a bootindex value already used should
>> fail with a friendly message rather than quitting a running VM.
>
> I think the problem is right where QEMU exits, i.e. in
> add_boot_device_path.  This function should return an error instead, via
> an Error ** argument.

Agree.

> Callers, typically a device's init or realize function, will either
> print the error before returning an error code (e.g. -EBUSY for init) or
> propagate the error up (for realize).
>
> Returning/propagating failure will still cause QEMU to exit when the
> duplicate bootindexes are found on the command line.

I have an unfinished patch in my tree that does exactly that.  It's
unfinished, because cleanup on error paths needs work.  Current state
appended with FIXMEs and all.  Beware, the FIXMEs may not be correct and
are almost certainly not complete.


diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index c5a6c21..f8b2b27 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2147,8 +2147,16 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    add_boot_device_path(isa->bootindexA, dev, "/floppy@0");
-    add_boot_device_path(isa->bootindexB, dev, "/floppy@1");
+    add_boot_device_path_err(isa->bootindexA, dev, "/floppy@0", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    add_boot_device_path_err(isa->bootindexB, dev, "/floppy@1", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
 }
 
 static void sysbus_fdc_initfn(Object *obj)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e2f55cc..de7ca31 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -678,6 +678,10 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
         return -1;
     }
 
+    if (add_boot_device_path(s->conf->bootindex, qdev, "/disk@0,0") < 0) {
+        return -1;
+    }
+
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
                 sizeof(struct virtio_blk_config));
 
@@ -691,6 +695,7 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     if (!virtio_blk_data_plane_create(vdev, blk, &s->dataplane)) {
         virtio_cleanup(vdev);
+        /* FIXME rm_boot_device_path() */
         return -1;
     }
     s->migration_state_notifier.notify = virtio_blk_migration_state_changed;
@@ -705,7 +710,6 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
 
     bdrv_iostatus_enable(s->bs);
 
-    add_boot_device_path(s->conf->bootindex, qdev, "/disk@0,0");
     return 0;
 }
 
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 7b3d3ee..c9568f5 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -687,12 +687,16 @@ int rom_add_file(const char *file, const char *fw_dir,
         snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
     }
 
-    add_boot_device_path(bootindex, NULL, devpath);
+    if (add_boot_device_path(bootindex, NULL, devpath) < 0) {
+        goto err_closed;
+        /* FIXME undo rom_insert()? */
+    }
     return 0;
 
 err:
     if (fd != -1)
         close(fd);
+err_closed:
     g_free(rom->data);
     g_free(rom->path);
     g_free(rom->name);
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 011764f..d532b21 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1813,9 +1813,7 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
 
     assigned_dev_load_option_rom(dev);
 
-    add_boot_device_path(dev->bootindex, &pci_dev->qdev, NULL);
-
-    return 0;
+    return add_boot_device_path(dev->bootindex, &pci_dev->qdev, NULL);
 
 assigned_out:
     deassign_device(dev);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 18c4b7e..557be55 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -166,10 +166,16 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
         return -1;
     }
 
+    if (add_boot_device_path(dev->conf.bootindex, &dev->qdev,
+                             dev->unit ? "/disk@1" : "/disk@0") < 0) {
+        return -1;
+    }
+
     if (ide_init_drive(s, dev->conf.bs, kind,
                        dev->version, dev->serial, dev->model, dev->wwn,
                        dev->conf.cyls, dev->conf.heads, dev->conf.secs,
                        dev->chs_trans) < 0) {
+        /* FIXME rm_boot_device_path() */
         return -1;
     }
 
@@ -180,9 +186,6 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
         dev->serial = g_strdup(s->drive_serial_str);
     }
 
-    add_boot_device_path(dev->conf.bootindex, &dev->qdev,
-                         dev->unit ? "/disk@1" : "/disk@0");
-
     return 0;
 }
 
diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index a1c08fb..9c064ce 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -3185,7 +3185,10 @@ static int vfio_initfn(PCIDevice *pdev)
         }
     }
 
-    add_boot_device_path(vdev->bootindex, &pdev->qdev, NULL);
+    if (add_boot_device_path(vdev->bootindex, &pdev->qdev, NULL) < 0) {
+        // FIXME cleanup
+        goto out_teardown;
+    }
     vfio_register_err_notifier(vdev);
 
     return 0;
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index d3f274c..ede2a35 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1490,7 +1490,10 @@ static int pci_e1000_init(PCIDevice *pci_dev)
 
     qemu_format_nic_info_str(qemu_get_queue(d->nic), macaddr);
 
-    add_boot_device_path(d->conf.bootindex, dev, "/ethernet-phy@0");
+    if (add_boot_device_path(d->conf.bootindex, dev, "/ethernet-phy@0") < 0) {
+        // FIXME cleanup
+        return -1;
+    }
 
     d->autoneg_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, e1000_autoneg_timer, d);
     d->mit_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, e1000_mit_timer, d);
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index ffa60d5..3cb986e 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1905,9 +1905,8 @@ static int e100_nic_init(PCIDevice *pci_dev)
     s->vmstate->name = qemu_get_queue(s->nic)->model;
     vmstate_register(&pci_dev->qdev, -1, s->vmstate, s);
 
-    add_boot_device_path(s->conf.bootindex, &pci_dev->qdev, "/ethernet-phy@0");
-
-    return 0;
+    return add_boot_device_path(s->conf.bootindex, &pci_dev->qdev,
+                                "/ethernet-phy@0");
 }
 
 static E100PCIDeviceInfo e100_devices[] = {
diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
index c961258..5541f2f 100644
--- a/hw/net/ne2000.c
+++ b/hw/net/ne2000.c
@@ -740,9 +740,8 @@ static int pci_ne2000_init(PCIDevice *pci_dev)
                           object_get_typename(OBJECT(pci_dev)), pci_dev->qdev.id, s);
     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->c.macaddr.a);
 
-    add_boot_device_path(s->c.bootindex, &pci_dev->qdev, "/ethernet-phy@0");
-
-    return 0;
+    return add_boot_device_path(s->c.bootindex, &pci_dev->qdev,
+                                "/ethernet-phy@0");
 }
 
 static void pci_ne2000_exit(PCIDevice *pci_dev)
diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 7cb47b3..66cac7c 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -1737,7 +1737,9 @@ int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info)
     s->nic = qemu_new_nic(info, &s->conf, object_get_typename(OBJECT(dev)), dev->id, s);
     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
 
-    add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0");
+    if (add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0") < 0) {
+        return -1;
+    }
 
     /* Initialize the PROM */
 
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index c31199f..9e5b648 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -3538,9 +3538,7 @@ static int pci_rtl8139_init(PCIDevice *dev)
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, rtl8139_timer, s);
     rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
 
-    add_boot_device_path(s->conf.bootindex, d, "/ethernet-phy@0");
-
-    return 0;
+    return add_boot_device_path(s->conf.bootindex, d, "/ethernet-phy@0");
 }
 
 static Property rtl8139_properties[] = {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index dd41008..020a8c1 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1564,8 +1564,7 @@ static int virtio_net_device_init(VirtIODevice *vdev)
     register_savevm(qdev, "virtio-net", -1, VIRTIO_NET_VM_VERSION,
                     virtio_net_save, virtio_net_load, n);
 
-    add_boot_device_path(n->nic_conf.bootindex, qdev, "/ethernet-phy@0");
-    return 0;
+    return add_boot_device_path(n->nic_conf.bootindex, qdev, "/ethernet-phy@0");
 }
 
 static int virtio_net_device_exit(DeviceState *qdev)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 49c2466..1da4c7b 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2106,9 +2106,7 @@ static int vmxnet3_pci_init(PCIDevice *pci_dev)
     register_savevm(dev, "vmxnet3-msix", -1, 1,
                     vmxnet3_msix_save, vmxnet3_msix_load, s);
 
-    add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0");
-
-    return 0;
+    return add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0");
 }
 
 
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 74e6a14..3450782 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2120,8 +2120,7 @@ static int scsi_initfn(SCSIDevice *dev)
     bdrv_set_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize);
 
     bdrv_iostatus_enable(s->qdev.conf.bs);
-    add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, NULL);
-    return 0;
+    return add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, NULL);
 }
 
 static int scsi_hd_initfn(SCSIDevice *dev)
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 8f195be..2e830e3 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -433,7 +433,9 @@ static int scsi_generic_initfn(SCSIDevice *s)
     s->type = scsiid.scsi_type;
     DPRINTF("device type %d\n", s->type);
     if (s->type == TYPE_DISK || s->type == TYPE_ROM) {
-        add_boot_device_path(s->conf.bootindex, &s->qdev, NULL);
+        if (add_boot_device_path(s->conf.bootindex, &s->qdev, NULL) < 0) {
+            return -1;
+        }
     }
 
     switch (s->type) {
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 660d774..07d75af 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1373,8 +1373,7 @@ static int usb_net_initfn(USBDevice *dev)
              s->conf.macaddr.a[5]);
     usb_desc_set_string(dev, STRING_ETHADDR, s->usbstring_mac);
 
-    add_boot_device_path(s->conf.bootindex, &dev->qdev, "/ethernet@0");
-    return 0;
+    return add_boot_device_path(s->conf.bootindex, &dev->qdev, "/ethernet@0");
 }
 
 static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 128955d..5724bb5 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -904,7 +904,9 @@ static int usb_host_initfn(USBDevice *udev)
     qemu_add_exit_notifier(&s->exit);
 
     QTAILQ_INSERT_TAIL(&hostdevs, s, next);
-    add_boot_device_path(s->bootindex, &udev->qdev, NULL);
+    if (add_boot_device_path(s->bootindex, &udev->qdev, NULL) < 0) {
+        return -1;
+    }
     usb_host_auto_check(NULL);
     return 0;
 }
diff --git a/hw/usb/host-linux.c b/hw/usb/host-linux.c
index 65cd3b4..c7a7bd0 100644
--- a/hw/usb/host-linux.c
+++ b/hw/usb/host-linux.c
@@ -1488,8 +1488,7 @@ static int usb_host_initfn(USBDevice *dev)
     if (s->match.bus_num != 0 && s->match.port != NULL) {
         usb_host_claim_port(s);
     }
-    add_boot_device_path(s->bootindex, &dev->qdev, NULL);
-    return 0;
+    return add_boot_device_path(s->bootindex, &dev->qdev, NULL);
 }
 
 static const VMStateDescription vmstate_usb_host = {
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 287a505..b4d01f6 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1314,7 +1314,9 @@ static int usbredir_initfn(USBDevice *udev)
                           usbredir_chardev_read, usbredir_chardev_event, dev);
 
     qemu_add_vm_change_state_handler(usbredir_vm_state_change, dev);
-    add_boot_device_path(dev->bootindex, &udev->qdev, NULL);
+    if (add_boot_device_path(dev->bootindex, &udev->qdev, NULL) < 0) {
+        return -1;
+    }
     return 0;
 }
 
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b1aa059..b033d97 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -178,8 +178,10 @@ void usb_info(Monitor *mon, const QDict *qdict);
 
 void rtc_change_mon_event(struct tm *tm);
 
-void add_boot_device_path(int32_t bootindex, DeviceState *dev,
-                          const char *suffix);
+void add_boot_device_path_err(int32_t bootindex, DeviceState *dev,
+                              const char *suffix, Error **errp);
+int add_boot_device_path(int32_t bootindex, DeviceState *dev,
+                         const char *suffix) QEMU_WARN_UNUSED_RESULT;
 char *get_boot_devices_list(size_t *size);
 
 DeviceState *get_boot_device(uint32_t position);
diff --git a/vl.c b/vl.c
index 4e709d5..18f9297 100644
--- a/vl.c
+++ b/vl.c
@@ -1158,8 +1158,8 @@ static void restore_boot_order(void *opaque)
     g_free(normal_boot_order);
 }
 
-void add_boot_device_path(int32_t bootindex, DeviceState *dev,
-                          const char *suffix)
+void add_boot_device_path_err(int32_t bootindex, DeviceState *dev,
+                              const char *suffix, Error **errp)
 {
     FWBootEntry *node, *i;
 
@@ -1176,8 +1176,9 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
 
     QTAILQ_FOREACH(i, &fw_boot_order, link) {
         if (i->bootindex == bootindex) {
-            fprintf(stderr, "Two devices with same boot index %d\n", bootindex);
-            exit(1);
+            error_setg(errp, "Two devices with same boot index %d",
+                       bootindex);
+            return;
         } else if (i->bootindex < bootindex) {
             continue;
         }
@@ -1187,6 +1188,20 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
     QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
 }
 
+int add_boot_device_path(int32_t bootindex, DeviceState *dev,
+                          const char *suffix)
+{
+    Error *local_err = NULL;
+
+    add_boot_device_path_err(bootindex, dev, suffix, &local_err);
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+    return 0;
+}
+
 DeviceState *get_boot_device(uint32_t position)
 {
     uint32_t counter = 0;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Marcel Apfelbaum]

On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
> >> Qemu is expected to quit if the same boot index value is used by two devices.
> >> However, hot-plugging a device with a bootindex value already used should
> >> fail with a friendly message rather than quitting a running VM.
> >
> > I think the problem is right where QEMU exits, i.e. in
> > add_boot_device_path.  This function should return an error instead, via
> > an Error ** argument.
> 
> Agree.
> 
> > Callers, typically a device's init or realize function, will either
> > print the error before returning an error code (e.g. -EBUSY for init) or
> > propagate the error up (for realize).
> >
> > Returning/propagating failure will still cause QEMU to exit when the
> > duplicate bootindexes are found on the command line.
> 
> I have an unfinished patch in my tree that does exactly that.  It's
> unfinished, because cleanup on error paths needs work.  Current state
> appended with FIXMEs and all.  Beware, the FIXMEs may not be correct and
> are almost certainly not complete.
Thanks Markus,
Should I use it as my starting point and finish it or you intend to?
Marcel

> 
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index c5a6c21..f8b2b27 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2147,8 +2147,16 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    add_boot_device_path(isa->bootindexA, dev, "/floppy@0");
> -    add_boot_device_path(isa->bootindexB, dev, "/floppy@1");
> +    add_boot_device_path_err(isa->bootindexA, dev, "/floppy@0", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    add_boot_device_path_err(isa->bootindexB, dev, "/floppy@1", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>  }
>  
>  static void sysbus_fdc_initfn(Object *obj)
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e2f55cc..de7ca31 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -678,6 +678,10 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
>          return -1;
>      }
>  
> +    if (add_boot_device_path(s->conf->bootindex, qdev, "/disk@0,0") < 0) {
> +        return -1;
> +    }
> +
>      virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
>                  sizeof(struct virtio_blk_config));
>  
> @@ -691,6 +695,7 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
>  #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
>      if (!virtio_blk_data_plane_create(vdev, blk, &s->dataplane)) {
>          virtio_cleanup(vdev);
> +        /* FIXME rm_boot_device_path() */
>          return -1;
>      }
>      s->migration_state_notifier.notify = virtio_blk_migration_state_changed;
> @@ -705,7 +710,6 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
>  
>      bdrv_iostatus_enable(s->bs);
>  
> -    add_boot_device_path(s->conf->bootindex, qdev, "/disk@0,0");
>      return 0;
>  }
>  
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 7b3d3ee..c9568f5 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -687,12 +687,16 @@ int rom_add_file(const char *file, const char *fw_dir,
>          snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
>      }
>  
> -    add_boot_device_path(bootindex, NULL, devpath);
> +    if (add_boot_device_path(bootindex, NULL, devpath) < 0) {
> +        goto err_closed;
> +        /* FIXME undo rom_insert()? */
> +    }
>      return 0;
>  
>  err:
>      if (fd != -1)
>          close(fd);
> +err_closed:
>      g_free(rom->data);
>      g_free(rom->path);
>      g_free(rom->name);
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index 011764f..d532b21 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -1813,9 +1813,7 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
>  
>      assigned_dev_load_option_rom(dev);
>  
> -    add_boot_device_path(dev->bootindex, &pci_dev->qdev, NULL);
> -
> -    return 0;
> +    return add_boot_device_path(dev->bootindex, &pci_dev->qdev, NULL);
>  
>  assigned_out:
>      deassign_device(dev);
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 18c4b7e..557be55 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -166,10 +166,16 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>          return -1;
>      }
>  
> +    if (add_boot_device_path(dev->conf.bootindex, &dev->qdev,
> +                             dev->unit ? "/disk@1" : "/disk@0") < 0) {
> +        return -1;
> +    }
> +
>      if (ide_init_drive(s, dev->conf.bs, kind,
>                         dev->version, dev->serial, dev->model, dev->wwn,
>                         dev->conf.cyls, dev->conf.heads, dev->conf.secs,
>                         dev->chs_trans) < 0) {
> +        /* FIXME rm_boot_device_path() */
>          return -1;
>      }
>  
> @@ -180,9 +186,6 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>          dev->serial = g_strdup(s->drive_serial_str);
>      }
>  
> -    add_boot_device_path(dev->conf.bootindex, &dev->qdev,
> -                         dev->unit ? "/disk@1" : "/disk@0");
> -
>      return 0;
>  }
>  
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index a1c08fb..9c064ce 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -3185,7 +3185,10 @@ static int vfio_initfn(PCIDevice *pdev)
>          }
>      }
>  
> -    add_boot_device_path(vdev->bootindex, &pdev->qdev, NULL);
> +    if (add_boot_device_path(vdev->bootindex, &pdev->qdev, NULL) < 0) {
> +        // FIXME cleanup
> +        goto out_teardown;
> +    }
>      vfio_register_err_notifier(vdev);
>  
>      return 0;
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index d3f274c..ede2a35 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -1490,7 +1490,10 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>  
>      qemu_format_nic_info_str(qemu_get_queue(d->nic), macaddr);
>  
> -    add_boot_device_path(d->conf.bootindex, dev, "/ethernet-phy@0");
> +    if (add_boot_device_path(d->conf.bootindex, dev, "/ethernet-phy@0") < 0) {
> +        // FIXME cleanup
> +        return -1;
> +    }
>  
>      d->autoneg_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, e1000_autoneg_timer, d);
>      d->mit_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, e1000_mit_timer, d);
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index ffa60d5..3cb986e 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -1905,9 +1905,8 @@ static int e100_nic_init(PCIDevice *pci_dev)
>      s->vmstate->name = qemu_get_queue(s->nic)->model;
>      vmstate_register(&pci_dev->qdev, -1, s->vmstate, s);
>  
> -    add_boot_device_path(s->conf.bootindex, &pci_dev->qdev, "/ethernet-phy@0");
> -
> -    return 0;
> +    return add_boot_device_path(s->conf.bootindex, &pci_dev->qdev,
> +                                "/ethernet-phy@0");
>  }
>  
>  static E100PCIDeviceInfo e100_devices[] = {
> diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
> index c961258..5541f2f 100644
> --- a/hw/net/ne2000.c
> +++ b/hw/net/ne2000.c
> @@ -740,9 +740,8 @@ static int pci_ne2000_init(PCIDevice *pci_dev)
>                            object_get_typename(OBJECT(pci_dev)), pci_dev->qdev.id, s);
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->c.macaddr.a);
>  
> -    add_boot_device_path(s->c.bootindex, &pci_dev->qdev, "/ethernet-phy@0");
> -
> -    return 0;
> +    return add_boot_device_path(s->c.bootindex, &pci_dev->qdev,
> +                                "/ethernet-phy@0");
>  }
>  
>  static void pci_ne2000_exit(PCIDevice *pci_dev)
> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
> index 7cb47b3..66cac7c 100644
> --- a/hw/net/pcnet.c
> +++ b/hw/net/pcnet.c
> @@ -1737,7 +1737,9 @@ int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info)
>      s->nic = qemu_new_nic(info, &s->conf, object_get_typename(OBJECT(dev)), dev->id, s);
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>  
> -    add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0");
> +    if (add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0") < 0) {
> +        return -1;
> +    }
>  
>      /* Initialize the PROM */
>  
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index c31199f..9e5b648 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -3538,9 +3538,7 @@ static int pci_rtl8139_init(PCIDevice *dev)
>      s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, rtl8139_timer, s);
>      rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>  
> -    add_boot_device_path(s->conf.bootindex, d, "/ethernet-phy@0");
> -
> -    return 0;
> +    return add_boot_device_path(s->conf.bootindex, d, "/ethernet-phy@0");
>  }
>  
>  static Property rtl8139_properties[] = {
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index dd41008..020a8c1 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1564,8 +1564,7 @@ static int virtio_net_device_init(VirtIODevice *vdev)
>      register_savevm(qdev, "virtio-net", -1, VIRTIO_NET_VM_VERSION,
>                      virtio_net_save, virtio_net_load, n);
>  
> -    add_boot_device_path(n->nic_conf.bootindex, qdev, "/ethernet-phy@0");
> -    return 0;
> +    return add_boot_device_path(n->nic_conf.bootindex, qdev, "/ethernet-phy@0");
>  }
>  
>  static int virtio_net_device_exit(DeviceState *qdev)
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 49c2466..1da4c7b 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2106,9 +2106,7 @@ static int vmxnet3_pci_init(PCIDevice *pci_dev)
>      register_savevm(dev, "vmxnet3-msix", -1, 1,
>                      vmxnet3_msix_save, vmxnet3_msix_load, s);
>  
> -    add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0");
> -
> -    return 0;
> +    return add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0");
>  }
>  
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 74e6a14..3450782 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2120,8 +2120,7 @@ static int scsi_initfn(SCSIDevice *dev)
>      bdrv_set_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize);
>  
>      bdrv_iostatus_enable(s->qdev.conf.bs);
> -    add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, NULL);
> -    return 0;
> +    return add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, NULL);
>  }
>  
>  static int scsi_hd_initfn(SCSIDevice *dev)
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 8f195be..2e830e3 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -433,7 +433,9 @@ static int scsi_generic_initfn(SCSIDevice *s)
>      s->type = scsiid.scsi_type;
>      DPRINTF("device type %d\n", s->type);
>      if (s->type == TYPE_DISK || s->type == TYPE_ROM) {
> -        add_boot_device_path(s->conf.bootindex, &s->qdev, NULL);
> +        if (add_boot_device_path(s->conf.bootindex, &s->qdev, NULL) < 0) {
> +            return -1;
> +        }
>      }
>  
>      switch (s->type) {
> diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
> index 660d774..07d75af 100644
> --- a/hw/usb/dev-network.c
> +++ b/hw/usb/dev-network.c
> @@ -1373,8 +1373,7 @@ static int usb_net_initfn(USBDevice *dev)
>               s->conf.macaddr.a[5]);
>      usb_desc_set_string(dev, STRING_ETHADDR, s->usbstring_mac);
>  
> -    add_boot_device_path(s->conf.bootindex, &dev->qdev, "/ethernet@0");
> -    return 0;
> +    return add_boot_device_path(s->conf.bootindex, &dev->qdev, "/ethernet@0");
>  }
>  
>  static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 128955d..5724bb5 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -904,7 +904,9 @@ static int usb_host_initfn(USBDevice *udev)
>      qemu_add_exit_notifier(&s->exit);
>  
>      QTAILQ_INSERT_TAIL(&hostdevs, s, next);
> -    add_boot_device_path(s->bootindex, &udev->qdev, NULL);
> +    if (add_boot_device_path(s->bootindex, &udev->qdev, NULL) < 0) {
> +        return -1;
> +    }
>      usb_host_auto_check(NULL);
>      return 0;
>  }
> diff --git a/hw/usb/host-linux.c b/hw/usb/host-linux.c
> index 65cd3b4..c7a7bd0 100644
> --- a/hw/usb/host-linux.c
> +++ b/hw/usb/host-linux.c
> @@ -1488,8 +1488,7 @@ static int usb_host_initfn(USBDevice *dev)
>      if (s->match.bus_num != 0 && s->match.port != NULL) {
>          usb_host_claim_port(s);
>      }
> -    add_boot_device_path(s->bootindex, &dev->qdev, NULL);
> -    return 0;
> +    return add_boot_device_path(s->bootindex, &dev->qdev, NULL);
>  }
>  
>  static const VMStateDescription vmstate_usb_host = {
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 287a505..b4d01f6 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1314,7 +1314,9 @@ static int usbredir_initfn(USBDevice *udev)
>                            usbredir_chardev_read, usbredir_chardev_event, dev);
>  
>      qemu_add_vm_change_state_handler(usbredir_vm_state_change, dev);
> -    add_boot_device_path(dev->bootindex, &udev->qdev, NULL);
> +    if (add_boot_device_path(dev->bootindex, &udev->qdev, NULL) < 0) {
> +        return -1;
> +    }
>      return 0;
>  }
>  
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b1aa059..b033d97 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -178,8 +178,10 @@ void usb_info(Monitor *mon, const QDict *qdict);
>  
>  void rtc_change_mon_event(struct tm *tm);
>  
> -void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> -                          const char *suffix);
> +void add_boot_device_path_err(int32_t bootindex, DeviceState *dev,
> +                              const char *suffix, Error **errp);
> +int add_boot_device_path(int32_t bootindex, DeviceState *dev,
> +                         const char *suffix) QEMU_WARN_UNUSED_RESULT;
>  char *get_boot_devices_list(size_t *size);
>  
>  DeviceState *get_boot_device(uint32_t position);
> diff --git a/vl.c b/vl.c
> index 4e709d5..18f9297 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1158,8 +1158,8 @@ static void restore_boot_order(void *opaque)
>      g_free(normal_boot_order);
>  }
>  
> -void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> -                          const char *suffix)
> +void add_boot_device_path_err(int32_t bootindex, DeviceState *dev,
> +                              const char *suffix, Error **errp)
>  {
>      FWBootEntry *node, *i;
>  
> @@ -1176,8 +1176,9 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>  
>      QTAILQ_FOREACH(i, &fw_boot_order, link) {
>          if (i->bootindex == bootindex) {
> -            fprintf(stderr, "Two devices with same boot index %d\n", bootindex);
> -            exit(1);
> +            error_setg(errp, "Two devices with same boot index %d",
> +                       bootindex);
> +            return;
>          } else if (i->bootindex < bootindex) {
>              continue;
>          }
> @@ -1187,6 +1188,20 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>      QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
>  }
>  
> +int add_boot_device_path(int32_t bootindex, DeviceState *dev,
> +                          const char *suffix)
> +{
> +    Error *local_err = NULL;
> +
> +    add_boot_device_path_err(bootindex, dev, suffix, &local_err);
> +    if (local_err) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
>  DeviceState *get_boot_device(uint32_t position)
>  {
>      uint32_t counter = 0;

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Markus Armbruster]

Marcel Apfelbaum <marcel.a@redhat.com> writes:

> On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
>> >> Qemu is expected to quit if the same boot index value is used by
>> >> two devices.
>> >> However, hot-plugging a device with a bootindex value already used should
>> >> fail with a friendly message rather than quitting a running VM.
>> >
>> > I think the problem is right where QEMU exits, i.e. in
>> > add_boot_device_path.  This function should return an error instead, via
>> > an Error ** argument.
>> 
>> Agree.
>> 
>> > Callers, typically a device's init or realize function, will either
>> > print the error before returning an error code (e.g. -EBUSY for init) or
>> > propagate the error up (for realize).
>> >
>> > Returning/propagating failure will still cause QEMU to exit when the
>> > duplicate bootindexes are found on the command line.
>> 
>> I have an unfinished patch in my tree that does exactly that.  It's
>> unfinished, because cleanup on error paths needs work.  Current state
>> appended with FIXMEs and all.  Beware, the FIXMEs may not be correct and
>> are almost certainly not complete.
> Thanks Markus,
> Should I use it as my starting point and finish it or you intend to?

If you have cycles to spare, you're quite welcome to take this patch and
run with it!

You may have noticed that my patch moves the code to add the boot device
path in a few cases.  I did this in the hope of simplifying error paths.
Do not hesitate to undo such moves where they turn out not to simplify
anything.

Here's an issue that exists before my patch: DeviceClass method
unrealize() should clean up everything done by realize().  In
particular, unrealize() needs to remove any entry added to fw_boot_order
by realize() via add_boot_device_path().  Code to do that doesn't exist,
yet.  Hot unplug is technically broken for all devices with bootindex.
Impact unknown.  Should probably be fixed in a separate patch.

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Marcel Apfelbaum]

On Thu, 2013-09-12 at 13:04 +0200, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> 
> >> > Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
> >> >> Qemu is expected to quit if the same boot index value is used by
> >> >> two devices.
> >> >> However, hot-plugging a device with a bootindex value already used should
> >> >> fail with a friendly message rather than quitting a running VM.
> >> >
> >> > I think the problem is right where QEMU exits, i.e. in
> >> > add_boot_device_path.  This function should return an error instead, via
> >> > an Error ** argument.
> >> 
> >> Agree.
> >> 
> >> > Callers, typically a device's init or realize function, will either
> >> > print the error before returning an error code (e.g. -EBUSY for init) or
> >> > propagate the error up (for realize).
> >> >
> >> > Returning/propagating failure will still cause QEMU to exit when the
> >> > duplicate bootindexes are found on the command line.
> >> 
> >> I have an unfinished patch in my tree that does exactly that.  It's
> >> unfinished, because cleanup on error paths needs work.  Current state
> >> appended with FIXMEs and all.  Beware, the FIXMEs may not be correct and
> >> are almost certainly not complete.
> > Thanks Markus,
> > Should I use it as my starting point and finish it or you intend to?
> 
> If you have cycles to spare, you're quite welcome to take this patch and
> run with it!
I'll try to finish it, thanks!
> 
> You may have noticed that my patch moves the code to add the boot device
> path in a few cases.  I did this in the hope of simplifying error paths.
> Do not hesitate to undo such moves where they turn out not to simplify
> anything.
> 
> Here's an issue that exists before my patch: DeviceClass method
> unrealize() should clean up everything done by realize().  In
> particular, unrealize() needs to remove any entry added to fw_boot_order
> by realize() via add_boot_device_path().  Code to do that doesn't exist,
> yet.  Hot unplug is technically broken for all devices with bootindex.
> Impact unknown.  Should probably be fixed in a separate patch.
The outcome is that after hot-unplugging a device with bootindex=x,
the device still remains in the fw_boot_order and no device can 
be hot-plugged with the same bootindex 'x'. Furthermore, because
of the current issue Qemu quits on an operation that should succeed! 
I plan to send a patch to fix this too.

Thanks,
Marcel
 

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Marcel Apfelbaum]

On Thu, 2013-09-12 at 13:04 +0200, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> 
> >> > Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
> >> >> Qemu is expected to quit if the same boot index value is used by
> >> >> two devices.
> >> >> However, hot-plugging a device with a bootindex value already used should
> >> >> fail with a friendly message rather than quitting a running VM.
> >> >
> >> > I think the problem is right where QEMU exits, i.e. in
> >> > add_boot_device_path.  This function should return an error instead, via
> >> > an Error ** argument.
> >> 
> >> Agree.

I understood that the boot order is passed in fw cfg and updated only once at
"machine done". There is no update of this list after this point.
Modifying the boot order from monitor does not work at all.

So in order to solve this issue we can:
1. Don't allow use of bootindex at hot-plug
2. Change the architecture so boot order changing during hot-plug will be possible.

> >> 
> >> > Callers, typically a device's init or realize function, will either
> >> > print the error before returning an error code (e.g. -EBUSY for init) or
> >> > propagate the error up (for realize).
> >> >
> >> > Returning/propagating failure will still cause QEMU to exit when the
> >> > duplicate bootindexes are found on the command line.
> >> 
> >> I have an unfinished patch in my tree that does exactly that.  It's
> >> unfinished, because cleanup on error paths needs work.  Current state
> >> appended with FIXMEs and all.  Beware, the FIXMEs may not be correct and
> >> are almost certainly not complete.
> > Thanks Markus,
> > Should I use it as my starting point and finish it or you intend to?
> 
> If you have cycles to spare, you're quite welcome to take this patch and
> run with it!
I really wanted to take this, but I think we need first to sort out
the issues mentioned above and only then follow this path

Thanks,
Marcel

> 
> You may have noticed that my patch moves the code to add the boot device
> path in a few cases.  I did this in the hope of simplifying error paths.
> Do not hesitate to undo such moves where they turn out not to simplify
> anything.
> 
> Here's an issue that exists before my patch: DeviceClass method
> unrealize() should clean up everything done by realize().  In
> particular, unrealize() needs to remove any entry added to fw_boot_order
> by realize() via add_boot_device_path().  Code to do that doesn't exist,
> yet.  Hot unplug is technically broken for all devices with bootindex.
> Impact unknown.  Should probably be fixed in a separate patch.

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Paolo Bonzini]

Il 16/09/2013 11:54, Marcel Apfelbaum ha scritto:
> On Thu, 2013-09-12 at 13:04 +0200, Markus Armbruster wrote:
>> Marcel Apfelbaum <marcel.a@redhat.com> writes:
>>
>>> On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote:
>>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>>
>>>>> Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
>>>>>> Qemu is expected to quit if the same boot index value is used by
>>>>>> two devices.
>>>>>> However, hot-plugging a device with a bootindex value already used should
>>>>>> fail with a friendly message rather than quitting a running VM.
>>>>>
>>>>> I think the problem is right where QEMU exits, i.e. in
>>>>> add_boot_device_path.  This function should return an error instead, via
>>>>> an Error ** argument.
>>>>
>>>> Agree.
> 
> I understood that the boot order is passed in fw cfg and updated only once at
> "machine done". There is no update of this list after this point.
> Modifying the boot order from monitor does not work at all.
> 
> So in order to solve this issue we can:
> 1. Don't allow use of bootindex at hot-plug
> 2. Change the architecture so boot order changing during hot-plug will be possible.

This is done relatively easily in add_boot_device_path (check the
qdev_hotplug variable and return an error if it is 1).

You can do it on top of Markus's patch.

Paolo

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Gleb Natapov]

On Mon, Sep 16, 2013 at 12:54:39PM +0300, Marcel Apfelbaum wrote:
> On Thu, 2013-09-12 at 13:04 +0200, Markus Armbruster wrote:
> > Marcel Apfelbaum <marcel.a@redhat.com> writes:
> > 
> > > On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote:
> > >> Paolo Bonzini <pbonzini@redhat.com> writes:
> > >> 
> > >> > Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
> > >> >> Qemu is expected to quit if the same boot index value is used by
> > >> >> two devices.
> > >> >> However, hot-plugging a device with a bootindex value already used should
> > >> >> fail with a friendly message rather than quitting a running VM.
> > >> >
> > >> > I think the problem is right where QEMU exits, i.e. in
> > >> > add_boot_device_path.  This function should return an error instead, via
> > >> > an Error ** argument.
> > >> 
> > >> Agree.
> 
> I understood that the boot order is passed in fw cfg and updated only once at
> "machine done". There is no update of this list after this point.
The reason it is done at his point is because when
add_boot_device_path() is called dev is not fully instantiated so
qdev_get_fw_dev_path() cannot be called on it yet. For hotplug we need
to re-create boot device list when device is fully ready.

> Modifying the boot order from monitor does not work at all.
> 
> So in order to solve this issue we can:
> 1. Don't allow use of bootindex at hot-plug
I'd rather have a proper fix then workaround. BTW this will change
qmp interface so a command that worked before (for some definition of
"worked") will start to fail. Markus proposed to ignore bootindex clash,
also simple solution and has no downside described above, but has others
that we discussed.

> 2. Change the architecture so boot order changing during hot-plug will be possible.
> 
This is an easy part of the problem though. The hard part how not to
exit when bootindex clash happens ans this is easy since nobody knows
how well device creation errors are handled by qdev.

--
			Gleb.

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Michael S. Tsirkin]

On Mon, Sep 16, 2013 at 12:11:35PM +0200, Paolo Bonzini wrote:
> Il 16/09/2013 11:54, Marcel Apfelbaum ha scritto:
> > On Thu, 2013-09-12 at 13:04 +0200, Markus Armbruster wrote:
> >> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> >>
> >>> On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote:
> >>>> Paolo Bonzini <pbonzini@redhat.com> writes:
> >>>>
> >>>>> Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
> >>>>>> Qemu is expected to quit if the same boot index value is used by
> >>>>>> two devices.
> >>>>>> However, hot-plugging a device with a bootindex value already used should
> >>>>>> fail with a friendly message rather than quitting a running VM.
> >>>>>
> >>>>> I think the problem is right where QEMU exits, i.e. in
> >>>>> add_boot_device_path.  This function should return an error instead, via
> >>>>> an Error ** argument.
> >>>>
> >>>> Agree.
> > 
> > I understood that the boot order is passed in fw cfg and updated only once at
> > "machine done". There is no update of this list after this point.
> > Modifying the boot order from monitor does not work at all.
> > 
> > So in order to solve this issue we can:
> > 1. Don't allow use of bootindex at hot-plug
> > 2. Change the architecture so boot order changing during hot-plug will be possible.
> 
> This is done relatively easily in add_boot_device_path (check the
> qdev_hotplug variable and return an error if it is 1).

This refers to 1) here? That's nice but it's not really all that helpful.

> You can do it on top of Markus's patch.
> 
> Paolo

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Paolo Bonzini]

Il 16/09/2013 17:14, Michael S. Tsirkin ha scritto:
> On Mon, Sep 16, 2013 at 12:11:35PM +0200, Paolo Bonzini wrote:
>> Il 16/09/2013 11:54, Marcel Apfelbaum ha scritto:
>>> On Thu, 2013-09-12 at 13:04 +0200, Markus Armbruster wrote:
>>>> Marcel Apfelbaum <marcel.a@redhat.com> writes:
>>>>
>>>>> On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote:
>>>>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>>>>
>>>>>>> Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
>>>>>>>> Qemu is expected to quit if the same boot index value is used by
>>>>>>>> two devices.
>>>>>>>> However, hot-plugging a device with a bootindex value already used should
>>>>>>>> fail with a friendly message rather than quitting a running VM.
>>>>>>>
>>>>>>> I think the problem is right where QEMU exits, i.e. in
>>>>>>> add_boot_device_path.  This function should return an error instead, via
>>>>>>> an Error ** argument.
>>>>>>
>>>>>> Agree.
>>>
>>> I understood that the boot order is passed in fw cfg and updated only once at
>>> "machine done". There is no update of this list after this point.
>>> Modifying the boot order from monitor does not work at all.
>>>
>>> So in order to solve this issue we can:
>>> 1. Don't allow use of bootindex at hot-plug
>>> 2. Change the architecture so boot order changing during hot-plug will be possible.
>>
>> This is done relatively easily in add_boot_device_path (check the
>> qdev_hotplug variable and return an error if it is 1).
> 
> This refers to 1) here? That's nice but it's not really all that helpful.

True, but the error propagation changes are the base for both cases, and
(1) is just 4 lines of code.  So this seems like a plan to me: do the
error propagation changes and (1), then we can revert those four lines
when (2) is ready.

Paolo

Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Michael S. Tsirkin]

On Mon, Sep 16, 2013 at 05:34:04PM +0200, Paolo Bonzini wrote:
> Il 16/09/2013 17:14, Michael S. Tsirkin ha scritto:
> > On Mon, Sep 16, 2013 at 12:11:35PM +0200, Paolo Bonzini wrote:
> >> Il 16/09/2013 11:54, Marcel Apfelbaum ha scritto:
> >>> On Thu, 2013-09-12 at 13:04 +0200, Markus Armbruster wrote:
> >>>> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> >>>>
> >>>>> On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote:
> >>>>>> Paolo Bonzini <pbonzini@redhat.com> writes:
> >>>>>>
> >>>>>>> Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
> >>>>>>>> Qemu is expected to quit if the same boot index value is used by
> >>>>>>>> two devices.
> >>>>>>>> However, hot-plugging a device with a bootindex value already used should
> >>>>>>>> fail with a friendly message rather than quitting a running VM.
> >>>>>>>
> >>>>>>> I think the problem is right where QEMU exits, i.e. in
> >>>>>>> add_boot_device_path.  This function should return an error instead, via
> >>>>>>> an Error ** argument.
> >>>>>>
> >>>>>> Agree.
> >>>
> >>> I understood that the boot order is passed in fw cfg and updated only once at
> >>> "machine done". There is no update of this list after this point.
> >>> Modifying the boot order from monitor does not work at all.
> >>>
> >>> So in order to solve this issue we can:
> >>> 1. Don't allow use of bootindex at hot-plug
> >>> 2. Change the architecture so boot order changing during hot-plug will be possible.
> >>
> >> This is done relatively easily in add_boot_device_path (check the
> >> qdev_hotplug variable and return an error if it is 1).
> > 
> > This refers to 1) here? That's nice but it's not really all that helpful.
> 
> True, but the error propagation changes are the base for both cases, and
> (1) is just 4 lines of code.  So this seems like a plan to me: do the
> error propagation changes and (1), then we can revert those four lines
> when (2) is ready.
> 
> Paolo

I'm fine with applying error propagation, but maybe not (1).

Basically if you accept incorrect code the result often
is that controbutor disappears and it's never completed,
others are discouraged from addressing the problem because
incorrect code created maintainance issues, and
a half-solved problem is a less interesting target
than an unsolved one.

So if a contributor basically says he does not want
to address the complete problem, we should weight
carefully whether we want to be stuck with
half a solution for a long time.

In this case, I don't think it's justified.

-- 
MST