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 <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.
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, "/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;