On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote: > Paolo Bonzini <pbonz...@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;