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

[Markus Armbruster]

Paolo Bonzini <address@hidden> 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, "/address@hidden");
-    add_boot_device_path(isa->bootindexB, dev, "/address@hidden");
+    add_boot_device_path_err(isa->bootindexA, dev, "/address@hidden", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    add_boot_device_path_err(isa->bootindexB, dev, "/address@hidden", &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, "/address@hidden,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, "/address@hidden,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 ? "/address@hidden" : 
"/address@hidden") < 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 ? "/address@hidden" : "/address@hidden");
-
     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, "/address@hidden");
+    if (add_boot_device_path(d->conf.bootindex, dev, "/address@hidden") < 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, "/address@hidden");
-
-    return 0;
+    return add_boot_device_path(s->conf.bootindex, &pci_dev->qdev,
+                                "/address@hidden");
 }
 
 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, "/address@hidden");
-
-    return 0;
+    return add_boot_device_path(s->c.bootindex, &pci_dev->qdev,
+                                "/address@hidden");
 }
 
 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, "/address@hidden");
+    if (add_boot_device_path(s->conf.bootindex, dev, "/address@hidden") < 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, "/address@hidden");
-
-    return 0;
+    return add_boot_device_path(s->conf.bootindex, d, "/address@hidden");
 }
 
 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, "/address@hidden");
-    return 0;
+    return add_boot_device_path(n->nic_conf.bootindex, qdev, 
"/address@hidden");
 }
 
 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, "/address@hidden");
-
-    return 0;
+    return add_boot_device_path(s->conf.bootindex, dev, "/address@hidden");
 }
 
 
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, "/address@hidden");
-    return 0;
+    return add_boot_device_path(s->conf.bootindex, &dev->qdev, 
"/address@hidden");
 }
 
 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