[Qemu-devel] [PATCH v4 0/8] modify boot order of guest, and take effect after rebooting

[Qemu-devel] [PATCH v4 0/8] modify boot order of guest, and take effect after rebooting

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Sometimes, we want to modify boot order of a guest, but no need to
shutdown it. We can call dynamic changing bootindex of a guest, which
can be assured taking effect just after the guest rebooting.

For example, in P2V scene, we boot a guest and then attach a
new system disk, for copying some thing. We want to assign the
new disk as the booting disk, which means its bootindex=1.

Different nics can be assigen different bootindex dynamically
also make sense.

The patchsets add one qmp interface, and add an fw_cfg_machine_reset()
to achieve it.

Steps of testing:

./qemu-system-x86_64 -enable-kvm -m 4096 -smp 4 -name redhat6.2 -drive \
file=/home/redhat6.2.img,if=none,id=drive-ide0-0-0 \
-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
-drive file=/home/RH-DVD1.iso,if=none,id=drive-ide0-0-1 \
-device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1,bootindex=4 \
-vnc 0.0.0.0:10 -netdev type=user,id=net0 \
-device virtio-net-pci,netdev=net0,bootindex=3,id=nic1 \
-netdev type=user,id=net1 -device e1000,netdev=net1,bootindex=2,id=nic2 \
-drive file=/home/virtio-disk.vfd,if=none,id=drive-fdc0-0-0,format=raw \
-device isa-fdc,driveA=drive-fdc0-0-0,id=floppy1,bootindexA=5 -monitor stdio
QEMU 2.0.93 monitor - type 'help' for more information
(qemu) info bootindex
id       bootindex       suffix
"floppy1"        5      "/floppy@0"
"ide0-0-1"       4      "/disk@1"
"nic1"   3      "/ethernet-phy@0"
"nic2"   2      "/ethernet-phy@0"
"ide0-0-0"       1      "/disk@0"
(qemu) set-bootindex ide0-0-1 1
The bootindex 1 has already been used
(qemu) set-bootindex ide0-0-1 6 "/disk@1"
(qemu) set-bootindex ide0-0-1 0
(qemu) system_reset
(qemu) set-bootindex ide0-0-1 1
The bootindex 1 has already been used
(qemu) set-bootindex nic1 0
The bootindex 0 has already been used
(qemu) set-bootindex ide0-0-1 -1
(qemu) set-bootindex nic1 0
(qemu) info bootindex
id       bootindex       suffix
"floppy1"        5      "/floppy@0"
"nic2"   2      "/ethernet-phy@0"
"ide0-0-0"       1      "/disk@0"
"nic1"   0      "/ethernet-phy@0"
(qemu) system_reset
(qemu) 

Changes since v3:
 - rework del_* and modify_* function, because of virtio devices' specialation.
   For example, virtio-net's id is NULL, and its parent virtio-net-pci's id was assigned.
   Though the global fw_boot_order stored the virtio-net device.
 - call dell_boot_device_path in each individual device avoiding waste resouce.
 - introduce qmp "query-bootindex" command
 - introcude hmp "info bootindex" command
 - Fixes by Eric's reviewing comments, thanks.

Changes since v2:
 *address Gerd's reviewing suggestion:
 - use the old entry's suffix, if the caller do not pass it in.
 - call del_boot_device_path() from device_finalize() instead
   of placing it into each individual device.

  Thanks Gerd.

Changes since v1:
 *rework by Gerd's suggestion:
 - split modify and del fw_boot_order for single function.
 - change modify bootindex's realization which simply lookup
   the device and modify the bootindex. if the new bootindex
   has already used by another device just throw an error.
 - change to del_boot_device_path(DeviceState *dev) and simply delete all
   entries belonging to the device.

Gonglei (8):
  bootindex: add modify_boot_device_path function
  bootindex: add del_boot_device_path function
  fw_cfg: add fw_cfg_machine_reset function
  bootindex: delete bootindex when device is removed
  qmp: add set-bootindex command
  qemu-monitor: HMP set-bootindex wrapper
  qmp: add query-bootindex command
  qemu-monitor: add HMP "info-bootindex" command

 hmp-commands.hx           |  17 +++++++
 hmp.c                     |  33 +++++++++++++
 hmp.h                     |   2 +
 hw/block/virtio-blk.c     |   1 +
 hw/i386/kvm/pci-assign.c  |   1 +
 hw/misc/vfio.c            |   1 +
 hw/net/e1000.c            |   1 +
 hw/net/eepro100.c         |   1 +
 hw/net/ne2000.c           |   1 +
 hw/net/rtl8139.c          |   1 +
 hw/net/virtio-net.c       |   1 +
 hw/net/vmxnet3.c          |   1 +
 hw/nvram/fw_cfg.c         |  54 ++++++++++++++++++---
 hw/scsi/scsi-generic.c    |   1 +
 hw/usb/dev-network.c      |   1 +
 hw/usb/host-libusb.c      |   1 +
 hw/usb/redirect.c         |   1 +
 include/hw/nvram/fw_cfg.h |   2 +
 include/sysemu/sysemu.h   |   3 ++
 monitor.c                 |   7 +++
 qapi-schema.json          |  46 ++++++++++++++++++
 qmp-commands.hx           |  66 +++++++++++++++++++++++++
 qmp.c                     |  17 +++++++
 vl.c                      | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 24 files changed, 372 insertions(+), 7 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v4 1/8] bootindex: add modify_boot_device_path function

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

When we want to change one device's bootindex, we
should lookup the device and change the bootindex.
it is simply that remove it from the global boot list,
then re-add it, sorted by new bootindex.

If the new bootindex has already used by another device
just throw an error.

Allow changing the existing bootindex entry only,
not support adding new boot entries.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 include/sysemu/sysemu.h |  2 ++
 vl.c                    | 68 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index d8539fd..e1b0659 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -209,6 +209,8 @@ void usb_info(Monitor *mon, const QDict *qdict);
 
 void add_boot_device_path(int32_t bootindex, DeviceState *dev,
                           const char *suffix);
+void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
+                             const char *suffix);
 char *get_boot_devices_list(size_t *size, bool ignore_suffixes);
 
 DeviceState *get_boot_device(uint32_t position);
diff --git a/vl.c b/vl.c
index fe451aa..825f9fd 100644
--- a/vl.c
+++ b/vl.c
@@ -1248,6 +1248,74 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
     QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
 }
 
+static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst)
+{
+    bool ret = false;
+    char *devpath_src = qdev_get_fw_dev_path(src);
+    char *devpath_dst = qdev_get_fw_dev_path(dst);
+
+    if (!strcmp(devpath_src, devpath_dst)) {
+        ret = true;
+    }
+
+    g_free(devpath_src);
+    g_free(devpath_dst);
+    return ret;
+}
+
+void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
+                             const char *suffix)
+{
+    FWBootEntry *i, *old_entry = NULL;
+
+    assert(dev != NULL || suffix != NULL);
+
+    if (bootindex >= 0) {
+        QTAILQ_FOREACH(i, &fw_boot_order, link) {
+            if (i->bootindex == bootindex) {
+                qerror_report(ERROR_CLASS_GENERIC_ERROR,
+                              "The bootindex %d has already been used",
+                              bootindex);
+                return;
+            }
+        }
+    }
+
+    QTAILQ_FOREACH(i, &fw_boot_order, link) {
+        /*
+         * Delete the same original dev, if devices havn't id property,
+         * we must check they fw_dev_path to identify them.
+         */
+        if ((i->dev->id && !strcmp(i->dev->id, dev->id)) ||
+            (!i->dev->id && is_same_fw_dev_path(i->dev, dev))) {
+            if (!suffix) {
+                QTAILQ_REMOVE(&fw_boot_order, i, link);
+                old_entry = i;
+
+                break;
+            } else if (i->suffix && !strcmp(suffix, i->suffix)) {
+                QTAILQ_REMOVE(&fw_boot_order, i, link);
+                old_entry = i;
+
+                break;
+            }
+        }
+    }
+
+    if (!old_entry) {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR,
+                      "The device(%s) havn't configured bootindex property"
+                      " or suffix don't match", dev->id);
+
+        return;
+    }
+
+    add_boot_device_path(bootindex, dev, old_entry->suffix);
+
+    g_free(old_entry->suffix);
+    g_free(old_entry);
+}
+
 DeviceState *get_boot_device(uint32_t position)
 {
     uint32_t counter = 0;
-- 
1.7.12.4

[Qemu-devel] [PATCH v4 2/8] bootindex: add del_boot_device_path function

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Introduce del_boot_device_path() to clean up fw_cfg content when
hot-unplugging a device that refers to a bootindex.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 include/sysemu/sysemu.h |  1 +
 vl.c                    | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index e1b0659..7a79ff4 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -209,6 +209,7 @@ void usb_info(Monitor *mon, const QDict *qdict);
 
 void add_boot_device_path(int32_t bootindex, DeviceState *dev,
                           const char *suffix);
+void del_boot_device_path(DeviceState *dev);
 void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
                              const char *suffix);
 char *get_boot_devices_list(size_t *size, bool ignore_suffixes);
diff --git a/vl.c b/vl.c
index 825f9fd..49328df 100644
--- a/vl.c
+++ b/vl.c
@@ -1263,6 +1263,26 @@ static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst)
     return ret;
 }
 
+void del_boot_device_path(DeviceState *dev)
+{
+    FWBootEntry *i;
+
+    assert(dev != NULL);
+
+    /* remove all entries of the assigned device */
+    QTAILQ_FOREACH(i, &fw_boot_order, link) {
+        if ((i->dev->id && dev->id &&
+            !strcmp(i->dev->id, dev->id)) ||
+            (!i->dev->id && is_same_fw_dev_path(i->dev, dev))) {
+            QTAILQ_REMOVE(&fw_boot_order, i, link);
+            g_free(i->suffix);
+            g_free(i);
+
+            break;
+        }
+    }
+}
+
 void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
                              const char *suffix)
 {
-- 
1.7.12.4

[Qemu-devel] [PATCH v4 3/8] fw_cfg: add fw_cfg_machine_reset function

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

We must assure that the changed bootindex can take effect
when guest is rebooted. So we introduce fw_cfg_machine_reset(),
which change the fw_cfg file's bootindex data using the new
global fw_boot_order list.

Signed-off-by: Chenliang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/nvram/fw_cfg.c         | 54 +++++++++++++++++++++++++++++++++++++++++------
 include/hw/nvram/fw_cfg.h |  2 ++
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b71d251..a24a44d 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -56,7 +56,6 @@ struct FWCfgState {
     FWCfgFiles *files;
     uint16_t cur_entry;
     uint32_t cur_offset;
-    Notifier machine_ready;
 };
 
 #define JPG_FILE 0
@@ -402,6 +401,26 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
     s->entries[arch][key].callback_opaque = callback_opaque;
 }
 
+static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
+                                              void *data, size_t len)
+{
+    void *ptr;
+    int arch = !!(key & FW_CFG_ARCH_LOCAL);
+
+    key &= FW_CFG_ENTRY_MASK;
+
+    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
+
+    /* return the old data to the function caller, avoid memory leak */
+    ptr = s->entries[arch][key].data;
+    s->entries[arch][key].data = data;
+    s->entries[arch][key].len = len;
+    s->entries[arch][key].callback_opaque = NULL;
+    s->entries[arch][key].callback = NULL;
+
+    return ptr;
+}
+
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
 {
     fw_cfg_add_bytes_read_callback(s, key, NULL, NULL, data, len);
@@ -499,13 +518,36 @@ void fw_cfg_add_file(FWCfgState *s,  const char *filename,
     fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len);
 }
 
-static void fw_cfg_machine_ready(struct Notifier *n, void *data)
+void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
+                        void *data, size_t len)
+{
+    int i, index;
+
+    assert(s->files);
+
+    index = be32_to_cpu(s->files->count);
+    assert(index < FW_CFG_FILE_SLOTS);
+
+    for (i = 0; i < index; i++) {
+        if (strcmp(filename, s->files->f[i].name) == 0) {
+            return fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
+                                     data, len);
+        }
+    }
+    /* add new one */
+    fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len);
+    return NULL;
+}
+
+static void fw_cfg_machine_reset(void *opaque)
 {
+    void *ptr;
     size_t len;
-    FWCfgState *s = container_of(n, FWCfgState, machine_ready);
+    FWCfgState *s = opaque;
     char *bootindex = get_boot_devices_list(&len, false);
 
-    fw_cfg_add_file(s, "bootorder", (uint8_t*)bootindex, len);
+    ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
+    g_free(ptr);
 }
 
 FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
@@ -542,9 +584,7 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
     fw_cfg_bootsplash(s);
     fw_cfg_reboot(s);
 
-    s->machine_ready.notify = fw_cfg_machine_ready;
-    qemu_add_machine_init_done_notifier(&s->machine_ready);
-
+    qemu_register_reset(fw_cfg_machine_reset, s);
     return s;
 }
 
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 72b1549..56e1ed7 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -76,6 +76,8 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
 void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
                               void *data, size_t len);
+void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
+                         size_t len);
 FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
                         hwaddr crl_addr, hwaddr data_addr);
 
-- 
1.7.12.4

[Qemu-devel] [PATCH v4 4/8] bootindex: delete bootindex when device is removed

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Device should be removed from global boot list when
it is hot-unplugged.

Signed-off-by: Chenliang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/block/virtio-blk.c    | 1 +
 hw/i386/kvm/pci-assign.c | 1 +
 hw/misc/vfio.c           | 1 +
 hw/net/e1000.c           | 1 +
 hw/net/eepro100.c        | 1 +
 hw/net/ne2000.c          | 1 +
 hw/net/rtl8139.c         | 1 +
 hw/net/virtio-net.c      | 1 +
 hw/net/vmxnet3.c         | 1 +
 hw/scsi/scsi-generic.c   | 1 +
 hw/usb/dev-network.c     | 1 +
 hw/usb/host-libusb.c     | 1 +
 hw/usb/redirect.c        | 1 +
 13 files changed, 13 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index c241c50..49813d5 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -786,6 +786,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBlock *s = VIRTIO_BLK(dev);
 
+    del_boot_device_path(dev);
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     remove_migration_state_change_notifier(&s->migration_state_notifier);
     virtio_blk_data_plane_destroy(s->dataplane);
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index de33657..1322479 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1853,6 +1853,7 @@ static void assigned_exitfn(struct PCIDevice *pci_dev)
 {
     AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
 
+    del_boot_device_path(&pci_dev->qdev);
     deassign_device(dev);
     free_assigned_device(dev);
 }
diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 0b9eba0..f891312 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -4304,6 +4304,7 @@ static void vfio_exitfn(PCIDevice *pdev)
     VFIODevice *vdev = DO_UPCAST(VFIODevice, pdev, pdev);
     VFIOGroup *group = vdev->group;
 
+    del_boot_device_path(&pdev->qdev);
     vfio_unregister_err_notifier(vdev);
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_disable_interrupts(vdev);
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 0fc29a0..fa4e858 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1492,6 +1492,7 @@ pci_e1000_uninit(PCIDevice *dev)
 {
     E1000State *d = E1000(dev);
 
+    del_boot_device_path(DEVICE(dev));
     timer_del(d->autoneg_timer);
     timer_free(d->autoneg_timer);
     timer_del(d->mit_timer);
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 3263e3f..62951e4 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1843,6 +1843,7 @@ static void pci_nic_uninit(PCIDevice *pci_dev)
 {
     EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
 
+    del_boot_device_path(&pci_dev->qdev);
     memory_region_destroy(&s->mmio_bar);
     memory_region_destroy(&s->io_bar);
     memory_region_destroy(&s->flash_bar);
diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
index d558b8c..62afa65 100644
--- a/hw/net/ne2000.c
+++ b/hw/net/ne2000.c
@@ -748,6 +748,7 @@ static void pci_ne2000_exit(PCIDevice *pci_dev)
     PCINE2000State *d = DO_UPCAST(PCINE2000State, dev, pci_dev);
     NE2000State *s = &d->ne2000;
 
+    del_boot_device_path(&pci_dev->qdev);
     memory_region_destroy(&s->io);
     qemu_del_nic(s->nic);
     qemu_free_irq(s->irq);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 90bc5ec..b34c91a 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -3462,6 +3462,7 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
 {
     RTL8139State *s = RTL8139(dev);
 
+    del_boot_device_path(DEVICE(dev));
     memory_region_destroy(&s->bar_io);
     memory_region_destroy(&s->bar_mem);
     if (s->cplus_txbuffer) {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 268eff9..0419575 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1654,6 +1654,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp)
     virtio_net_set_status(vdev, 0);
 
     unregister_savevm(dev, "virtio-net", n);
+    del_boot_device_path(dev);
 
     g_free(n->netclient_name);
     n->netclient_name = NULL;
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 77bea6f..c10d84e 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2176,6 +2176,7 @@ static void vmxnet3_pci_uninit(PCIDevice *pci_dev)
     VMW_CBPRN("Starting uninit...");
 
     unregister_savevm(dev, "vmxnet3-msix", s);
+    del_boot_device_path(dev);
 
     vmxnet3_net_uninit(s);
 
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 3733d2c..b270bc7 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -388,6 +388,7 @@ static void scsi_generic_reset(DeviceState *dev)
 
 static void scsi_destroy(SCSIDevice *s)
 {
+    del_boot_device_path(&s->qdev);
     scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE));
     blockdev_mark_auto_del(s->conf.bs);
 }
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 518d536..be39802 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1331,6 +1331,7 @@ static void usb_net_handle_destroy(USBDevice *dev)
     /* TODO: remove the nd_table[] entry */
     rndis_clear_responsequeue(s);
     qemu_del_nic(s->nic);
+    del_boot_device_path(&dev->qdev);
 }
 
 static NetClientInfo net_usbnet_info = {
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index c189147..919a0d2 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -984,6 +984,7 @@ static void usb_host_handle_destroy(USBDevice *udev)
 {
     USBHostDevice *s = USB_HOST_DEVICE(udev);
 
+    del_boot_device_path(&udev->qdev);
     qemu_remove_exit_notifier(&s->exit);
     QTAILQ_REMOVE(&hostdevs, s, next);
     usb_host_close(s);
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 44522d9..33df3af 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1416,6 +1416,7 @@ static void usbredir_handle_destroy(USBDevice *udev)
 {
     USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
 
+    del_boot_device_path(&udev->qdev);
     qemu_chr_delete(dev->cs);
     dev->cs = NULL;
     /* Note must be done after qemu_chr_close, as that causes a close event */
-- 
1.7.12.4

[Qemu-devel] [PATCH v4 5/8] qmp: add set-bootindex command

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.

Example QMP command:
-> { "execute": "set-bootindex",
     "arguments": { "id": "ide0-0-1", "bootindex": 1, "suffix": "/disk@0"}}
<- { "return": {} }

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 qapi-schema.json | 17 +++++++++++++++++
 qmp-commands.hx  | 25 +++++++++++++++++++++++++
 qmp.c            | 17 +++++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index b11aad2..30bd6ad 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1704,6 +1704,23 @@
 { 'command': 'device_del', 'data': {'id': 'str'} }
 
 ##
+# @set-bootindex:
+#
+# set bootindex of a device
+#
+# @id: the name of the device
+# @bootindex: the bootindex of the device
+# @suffix: #optional a suffix of the device
+#
+# Returns: Nothing on success
+#          If @id is not a valid device, DeviceNotFound
+#
+# Since: 2.2
+##
+{ 'command': 'set-bootindex',
+  'data': {'id': 'str', 'bootindex': 'int', '*suffix': 'str'} }
+
+##
 # @DumpGuestMemoryFormat:
 #
 # An enumeration of guest-memory-dump's format.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4be4765..19cc3b8 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -330,6 +330,31 @@ Example:
 <- { "return": {} }
 
 EQMP
+    {
+        .name       = "set-bootindex",
+        .args_type  = "id:s,bootindex:l,suffix:s?",
+        .mhandler.cmd_new = qmp_marshal_input_set_bootindex,
+    },
+
+SQMP
+set-bootindex
+-------------
+
+Set bootindex of a device
+
+Arguments:
+
+- "id": the device's ID (json-string)
+- "bootindex": the device's bootindex (json-int)
+- "suffix": the device's suffix in global boot list (json-string, optional)
+
+Example:
+
+-> { "execute": "set-bootindex",
+     "arguments": { "id": "ide0-0-1", "bootindex": 1, "suffix": "/disk@0"}}
+<- { "return": {} }
+
+EQMP
 
     {
         .name       = "send-key",
diff --git a/qmp.c b/qmp.c
index 0d2553a..e046200 100644
--- a/qmp.c
+++ b/qmp.c
@@ -684,6 +684,23 @@ void qmp_object_del(const char *id, Error **errp)
     object_unparent(obj);
 }
 
+void qmp_set_bootindex(const char *id, int64_t bootindex,
+                       bool has_suffix, const char *suffix, Error **errp)
+{
+    DeviceState *dev;
+
+    dev = qdev_find_recursive(sysbus_get_default(), id);
+    if (!dev) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, id);
+        return;
+    }
+    if (has_suffix) {
+        modify_boot_device_path(bootindex, dev, suffix);
+    } else {
+        modify_boot_device_path(bootindex, dev, NULL);
+    }
+}
+
 MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
 {
     MemoryDeviceInfoList *head = NULL;
-- 
1.7.12.4

[Qemu-devel] [PATCH v4 6/8] qemu-monitor: HMP set-bootindex wrapper

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Add HMP set-bootindex wrapper to allow setting
devcie's bootindex via monitor.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 hmp-commands.hx | 15 +++++++++++++++
 hmp.c           | 13 +++++++++++++
 hmp.h           |  1 +
 3 files changed, 29 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d0943b1..31ef24e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -688,6 +688,21 @@ Remove device @var{id}.
 ETEXI
 
     {
+        .name       = "set-bootindex",
+        .args_type  = "id:s,bootindex:l,suffix:s?",
+        .params     = "device bootindex [suffix]",
+        .help       = "set bootindex of a device(e.g. set-bootindex disk0 1 '/disk@0')",
+        .mhandler.cmd = hmp_set_bootindex,
+    },
+
+STEXI
+@item set-bootindex @var{id} @var{bootindex}
+@findex set-bootindex
+
+Set bootindex of a device.
+ETEXI
+
+    {
         .name       = "cpu",
         .args_type  = "index:i",
         .params     = "index",
diff --git a/hmp.c b/hmp.c
index 4d1838e..95f7eeb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1714,3 +1714,16 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
 
     monitor_printf(mon, "\n");
 }
+
+void hmp_set_bootindex(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+
+    const char *id = qdict_get_str(qdict, "id");
+    int64_t bootindex = qdict_get_int(qdict, "bootindex");
+    bool has_suffix = qdict_haskey(qdict, "suffix");
+    const char *suffix = qdict_get_try_str(qdict, "suffix");
+
+    qmp_set_bootindex(id, bootindex, has_suffix, suffix, &err);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 4fd3c4a..eb2641a 100644
--- a/hmp.h
+++ b/hmp.h
@@ -94,6 +94,7 @@ void hmp_cpu_add(Monitor *mon, const QDict *qdict);
 void hmp_object_add(Monitor *mon, const QDict *qdict);
 void hmp_object_del(Monitor *mon, const QDict *qdict);
 void hmp_info_memdev(Monitor *mon, const QDict *qdict);
+void hmp_set_bootindex(Monitor *mon, const QDict *qdict);
 void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
 void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
 void device_add_completion(ReadLineState *rs, int nb_args, const char *str);
-- 
1.7.12.4

[Qemu-devel] [PATCH v4 7/8] qmp: add query-bootindex command

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Adds "query-bootindex" QMP command.

Example QMP command:

-> { "execute": "query-bootindex"}
<- {
      "return":[
         {
            "id":"ide0-0-0",
            "bootindex":1,
            "suffix":"/disk@0"
         },
         {
            "id":"nic1",
            "bootindex":2,
            "suffix":"/ethernet-phy@0"
         }
      ]
   }

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 qapi-schema.json | 29 +++++++++++++++++++++++++++++
 qmp-commands.hx  | 41 +++++++++++++++++++++++++++++++++++++++++
 vl.c             | 31 +++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index 30bd6ad..680cbc5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1704,6 +1704,33 @@
 { 'command': 'device_del', 'data': {'id': 'str'} }
 
 ##
+# @BootindexInfo:
+#
+# Information about devcie's bootindex.
+#
+# @id: the name of a device owning the bootindex
+#
+# @bootindex: the bootindex number
+#
+# @suffix: the suffix a device's bootindex
+#
+# Since: 2.2
+##
+{ 'type': 'BootindexInfo',
+  'data': {'id': 'str', 'bootindex': 'int', 'suffix': 'str'} }
+
+##
+# @query-bootindex:
+#
+# Returns information about bootindex
+#
+# Returns: a list of @BootindexInfo for existing device
+#
+# Since: 2.2
+##
+{ 'command': 'query-bootindex', 'returns': ['BootindexInfo'] }
+
+##
 # @set-bootindex:
 #
 # set bootindex of a device
@@ -1715,6 +1742,8 @@
 # Returns: Nothing on success
 #          If @id is not a valid device, DeviceNotFound
 #
+# Note: suffix can be gotten by query-bootindex command
+#
 # Since: 2.2
 ##
 { 'command': 'set-bootindex',
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 19cc3b8..6ab9325 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -337,6 +337,47 @@ EQMP
     },
 
 SQMP
+query-bootindex
+---------------
+
+Show VM bootindex information.
+
+The returned value is a json-array of all device configured
+bootindex property. Each bootindex is represented by a json-object.
+
+The bootindex json-object contains the following:
+
+- "id": the name of a device owning the bootindex (json-string)
+- "bootindex": the bootindex number (json-int)
+- "suffix": the suffix a device's bootindex (json-string)
+
+Example:
+
+-> { "execute": "query-bootindex" }
+<- {
+      "return":[
+         {
+            "id":"ide0-0-0",
+            "bootindex":1,
+            "suffix":"/disk@0"
+         },
+         {
+            "id":"nic1",
+            "bootindex":2,
+            "suffix":"/ethernet-phy@0"
+         }
+      ]
+   }
+
+EQMP
+
+    {
+        .name       = "query-bootindex",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_bootindex,
+    },
+
+SQMP
 set-bootindex
 -------------
 
diff --git a/vl.c b/vl.c
index 49328df..52e4d9a 100644
--- a/vl.c
+++ b/vl.c
@@ -1219,6 +1219,37 @@ static void restore_boot_order(void *opaque)
     g_free(normal_boot_order);
 }
 
+BootindexInfoList *qmp_query_bootindex(Error **errp)
+{
+    BootindexInfoList *booindex_list = NULL;
+    BootindexInfoList *info;
+    FWBootEntry *i;
+
+    QTAILQ_FOREACH(i, &fw_boot_order, link) {
+        info = g_new0(BootindexInfoList, 1);
+        info->value = g_new0(BootindexInfo, 1);
+
+        if (i->dev->id) {
+            info->value->id = g_strdup(i->dev->id);
+        } else {
+            /* For virtio devices, we should get id from they parent */
+            if (i->dev->parent_bus) {
+                DeviceState *dev = i->dev->parent_bus->parent;
+                info->value->id = g_strdup(dev->id);
+            } else {
+                info->value->id = g_strdup("");
+            }
+        }
+        info->value->bootindex = i->bootindex;
+        info->value->suffix = g_strdup(i->suffix);
+
+        info->next = booindex_list;
+        booindex_list = info;
+    }
+
+    return booindex_list;
+}
+
 void add_boot_device_path(int32_t bootindex, DeviceState *dev,
                           const char *suffix)
 {
-- 
1.7.12.4

[Qemu-devel] [PATCH v4 8/8] qemu-monitor: add HMP "info-bootindex" command

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Add HMP info-bootindex command to getting
devcie's bootindex via monitor.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 hmp-commands.hx |  2 ++
 hmp.c           | 20 ++++++++++++++++++++
 hmp.h           |  1 +
 monitor.c       |  7 +++++++
 4 files changed, 30 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 31ef24e..bc1b982 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1795,6 +1795,8 @@ show qdev device model list
 show roms
 @item info tpm
 show the TPM device
+@item info bootindex
+show the current VM bootindex information
 @end table
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index 95f7eeb..1688e02 100644
--- a/hmp.c
+++ b/hmp.c
@@ -725,6 +725,26 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
     qapi_free_TPMInfoList(info_list);
 }
 
+void hmp_info_bootindex(Monitor *mon, const QDict *qdict)
+{
+    BootindexInfoList *bootindex_list, *info;
+
+    bootindex_list = qmp_query_bootindex(NULL);
+    if (!bootindex_list) {
+        monitor_printf(mon, "No bootindex was configured\n");
+        return;
+    }
+
+    monitor_printf(mon, "id \t bootindex \t suffix\n");
+    for (info = bootindex_list; info; info = info->next) {
+        monitor_printf(mon, "\"%s\"\t %"PRId64"\t\"%s\"\n",
+                       info->value->id, info->value->bootindex,
+                       info->value->suffix);
+    }
+
+    qapi_free_BootindexInfoList(bootindex_list);
+}
+
 void hmp_quit(Monitor *mon, const QDict *qdict)
 {
     monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index eb2641a..5899537 100644
--- a/hmp.h
+++ b/hmp.h
@@ -38,6 +38,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
 void hmp_info_pci(Monitor *mon, const QDict *qdict);
 void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
 void hmp_info_tpm(Monitor *mon, const QDict *qdict);
+void hmp_info_bootindex(Monitor *mon, const QDict *qdict);
 void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index 5bc70a6..8158ddb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2918,6 +2918,13 @@ static mon_cmd_t info_cmds[] = {
         .mhandler.cmd = hmp_info_memdev,
     },
     {
+        .name       = "bootindex",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show the current VM bootindex information",
+        .mhandler.cmd = hmp_info_bootindex,
+    },
+    {
         .name       = NULL,
     },
 };
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v4 1/8] bootindex: add modify_boot_device_path function

[Eduardo Habkost]

On Thu, Jul 31, 2014 at 05:47:26PM +0800, arei.gonglei@huawei.com wrote:
[...]
> +void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
> +                             const char *suffix)
> +{
> +    FWBootEntry *i, *old_entry = NULL;
> +
> +    assert(dev != NULL || suffix != NULL);
> +
> +    if (bootindex >= 0) {
> +        QTAILQ_FOREACH(i, &fw_boot_order, link) {
> +            if (i->bootindex == bootindex) {
> +                qerror_report(ERROR_CLASS_GENERIC_ERROR,
> +                              "The bootindex %d has already been used",
> +                              bootindex);

Isn't an Error** parameter preferable here, instead of using qerror_report()?

> +                return;
> +            }
> +        }
> +    }
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 1/8] bootindex: add modify_boot_device_path function

[Luiz Capitulino]

On Fri, 1 Aug 2014 10:36:18 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jul 31, 2014 at 05:47:26PM +0800, arei.gonglei@huawei.com wrote:
> [...]
> > +void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
> > +                             const char *suffix)
> > +{
> > +    FWBootEntry *i, *old_entry = NULL;
> > +
> > +    assert(dev != NULL || suffix != NULL);
> > +
> > +    if (bootindex >= 0) {
> > +        QTAILQ_FOREACH(i, &fw_boot_order, link) {
> > +            if (i->bootindex == bootindex) {
> > +                qerror_report(ERROR_CLASS_GENERIC_ERROR,
> > +                              "The bootindex %d has already been used",
> > +                              bootindex);
> 
> Isn't an Error** parameter preferable here, instead of using qerror_report()?

Good catch. I'm not following this series, but using qerror_report() is
almost always wrong these days.

Re: [Qemu-devel] [PATCH v4 4/8] bootindex: delete bootindex when device is removed

[Eduardo Habkost]

On Thu, Jul 31, 2014 at 05:47:29PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Device should be removed from global boot list when
> it is hot-unplugged.
> 
> Signed-off-by: Chenliang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/block/virtio-blk.c    | 1 +
>  hw/i386/kvm/pci-assign.c | 1 +
>  hw/misc/vfio.c           | 1 +
>  hw/net/e1000.c           | 1 +
>  hw/net/eepro100.c        | 1 +
>  hw/net/ne2000.c          | 1 +
>  hw/net/rtl8139.c         | 1 +
>  hw/net/virtio-net.c      | 1 +
>  hw/net/vmxnet3.c         | 1 +
>  hw/scsi/scsi-generic.c   | 1 +
>  hw/usb/dev-network.c     | 1 +
>  hw/usb/host-libusb.c     | 1 +
>  hw/usb/redirect.c        | 1 +
>  13 files changed, 13 insertions(+)

Grepping for add_boot_device_path, I don't see corresponding
del_boot_device_path() calls in this patch for the following:

  hw/ide/qdev.c:    add_boot_device_path(dev->conf.bootindex, &dev->qdev,
  hw/block/fdc.c:    add_boot_device_path(isa->bootindexA, dev, "/floppy@0");
  hw/block/fdc.c:    add_boot_device_path(isa->bootindexB, dev, "/floppy@1");
  hw/net/pcnet.c:    add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0");
  hw/net/spapr_llan.c:    add_boot_device_path(dev->nicconf.bootindex, DEVICE(dev), "");
  hw/scsi/scsi-disk.c:    add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, NULL);

Why we don't need del_boot_device_path() calls for those?

These seem to be OK, and are handled by this patch:

  hw/i386/kvm/pci-assign.c:    add_boot_device_path(dev->bootindex, &pci_dev->qdev, NULL);
  hw/block/virtio-blk.c:    add_boot_device_path(s->conf->bootindex, dev, "/disk@0,0");
  hw/misc/vfio.c:    add_boot_device_path(vdev->bootindex, &pdev->qdev, NULL);
  hw/net/rtl8139.c:    add_boot_device_path(s->conf.bootindex, d, "/ethernet-phy@0");
  hw/net/e1000.c:    add_boot_device_path(d->conf.bootindex, dev, "/ethernet-phy@0");
  hw/net/eepro100.c:    add_boot_device_path(s->conf.bootindex, &pci_dev->qdev, "/ethernet-phy@0");
  hw/net/ne2000.c:    add_boot_device_path(s->c.bootindex, &pci_dev->qdev, "/ethernet-phy@0");
  hw/net/virtio-net.c:    add_boot_device_path(n->nic_conf.bootindex, dev, "/ethernet-phy@0");
  hw/net/vmxnet3.c:    add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0");
  hw/scsi/scsi-generic.c:        add_boot_device_path(s->conf.bootindex, &s->qdev, NULL);
  hw/usb/dev-network.c:    add_boot_device_path(s->conf.bootindex, &dev->qdev, "/ethernet@0");
  hw/usb/host-libusb.c:    add_boot_device_path(s->bootindex, &udev->qdev, NULL);
  hw/usb/redirect.c:    add_boot_device_path(dev->bootindex, &udev->qdev, NULL);

This one has dev==NULL, so it looks OK:

  hw/core/loader.c:    add_boot_device_path(bootindex, NULL, devpath);

This is modify_boot_device_path(), so it's OK:

  vl.c:    add_boot_device_path(bootindex, dev, old_entry->suffix);

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 4/8] bootindex: delete bootindex when device is removed

[Gonglei (Arei)]

Best regards,
-Gonglei


> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Friday, August 01, 2014 10:46 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; chenliang (T); Huangweidong (C);
> mst@redhat.com; aik@ozlabs.ru; hutao@cn.fujitsu.com;
> armbru@redhat.com; kraxel@redhat.com; akong@redhat.com;
> agraf@suse.de; aliguori@amazon.com; gaowanlong@cn.fujitsu.com;
> Luonengjun; Huangpeng (Peter); hani@linux.com; stefanha@redhat.com;
> pbonzini@redhat.com; lcapitulino@redhat.com; kwolf@redhat.com;
> peter.crosthwaite@xilinx.com; imammedo@redhat.com; afaerber@suse.de
> Subject: Re: [Qemu-devel] [PATCH v4 4/8] bootindex: delete bootindex when
> device is removed
> 
> On Thu, Jul 31, 2014 at 05:47:29PM +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > Device should be removed from global boot list when
> > it is hot-unplugged.
> >
> > Signed-off-by: Chenliang <chenliang88@huawei.com>
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/block/virtio-blk.c    | 1 +
> >  hw/i386/kvm/pci-assign.c | 1 +
> >  hw/misc/vfio.c           | 1 +
> >  hw/net/e1000.c           | 1 +
> >  hw/net/eepro100.c        | 1 +
> >  hw/net/ne2000.c          | 1 +
> >  hw/net/rtl8139.c         | 1 +
> >  hw/net/virtio-net.c      | 1 +
> >  hw/net/vmxnet3.c         | 1 +
> >  hw/scsi/scsi-generic.c   | 1 +
> >  hw/usb/dev-network.c     | 1 +
> >  hw/usb/host-libusb.c     | 1 +
> >  hw/usb/redirect.c        | 1 +
> >  13 files changed, 13 insertions(+)
> 
> Grepping for add_boot_device_path, I don't see corresponding
> del_boot_device_path() calls in this patch for the following:
> 
>   hw/ide/qdev.c:    add_boot_device_path(dev->conf.bootindex,
> &dev->qdev,
>   hw/block/fdc.c:    add_boot_device_path(isa->bootindexA, dev,
> "/floppy@0");
>   hw/block/fdc.c:    add_boot_device_path(isa->bootindexB, dev,
> "/floppy@1");
>   hw/net/pcnet.c:    add_boot_device_path(s->conf.bootindex, dev,
> "/ethernet-phy@0");
>   hw/net/spapr_llan.c:    add_boot_device_path(dev->nicconf.bootindex,
> DEVICE(dev), "");
>   hw/scsi/scsi-disk.c:    add_boot_device_path(s->qdev.conf.bootindex,
> &dev->qdev, NULL);
> 
> Why we don't need del_boot_device_path() calls for those?
> 
I think those device don't support hot-plug/hot-unplug. So, needn't call 
del_boot_device_path(). But maybe I make a mistake for pcnet and scsi-disk.

I will adopt Gerd's suggestion in v5: 
call this from device_finalize() instead of placing it into each
individual device.

Thanks.

> These seem to be OK, and are handled by this patch:
> 
>   hw/i386/kvm/pci-assign.c:    add_boot_device_path(dev->bootindex,
> &pci_dev->qdev, NULL);
>   hw/block/virtio-blk.c:    add_boot_device_path(s->conf->bootindex, dev,
> "/disk@0,0");
>   hw/misc/vfio.c:    add_boot_device_path(vdev->bootindex, &pdev->qdev,
> NULL);
>   hw/net/rtl8139.c:    add_boot_device_path(s->conf.bootindex, d,
> "/ethernet-phy@0");
>   hw/net/e1000.c:    add_boot_device_path(d->conf.bootindex, dev,
> "/ethernet-phy@0");
>   hw/net/eepro100.c:    add_boot_device_path(s->conf.bootindex,
> &pci_dev->qdev, "/ethernet-phy@0");
>   hw/net/ne2000.c:    add_boot_device_path(s->c.bootindex,
> &pci_dev->qdev, "/ethernet-phy@0");
>   hw/net/virtio-net.c:    add_boot_device_path(n->nic_conf.bootindex, dev,
> "/ethernet-phy@0");
>   hw/net/vmxnet3.c:    add_boot_device_path(s->conf.bootindex, dev,
> "/ethernet-phy@0");
>   hw/scsi/scsi-generic.c:        add_boot_device_path(s->conf.bootindex,
> &s->qdev, NULL);
>   hw/usb/dev-network.c:    add_boot_device_path(s->conf.bootindex,
> &dev->qdev, "/ethernet@0");
>   hw/usb/host-libusb.c:    add_boot_device_path(s->bootindex,
> &udev->qdev, NULL);
>   hw/usb/redirect.c:    add_boot_device_path(dev->bootindex,
> &udev->qdev, NULL);
> 
> This one has dev==NULL, so it looks OK:
> 
>   hw/core/loader.c:    add_boot_device_path(bootindex, NULL, devpath);
> 
> This is modify_boot_device_path(), so it's OK:
> 
>   vl.c:    add_boot_device_path(bootindex, dev, old_entry->suffix);
> 
> --
> Eduardo

Re: [Qemu-devel] [PATCH v4 1/8] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

Hi,

> 
> > On Thu, Jul 31, 2014 at 05:47:26PM +0800, arei.gonglei@huawei.com wrote:
> > [...]
> > > +void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
> > > +                             const char *suffix)
> > > +{
> > > +    FWBootEntry *i, *old_entry = NULL;
> > > +
> > > +    assert(dev != NULL || suffix != NULL);
> > > +
> > > +    if (bootindex >= 0) {
> > > +        QTAILQ_FOREACH(i, &fw_boot_order, link) {
> > > +            if (i->bootindex == bootindex) {
> > > +                qerror_report(ERROR_CLASS_GENERIC_ERROR,
> > > +                              "The bootindex %d has already been
> used",
> > > +                              bootindex);
> >
> > Isn't an Error** parameter preferable here, instead of using qerror_report()?
> 
> Good catch. I'm not following this series, but using qerror_report() is
> almost always wrong these days.
>
Would you give me some advice? Thanks.
Using error_report() instead of qerror_report()?

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v4 1/8] bootindex: add modify_boot_device_path function

[Markus Armbruster]

"Gonglei (Arei)" <arei.gonglei@huawei.com> writes:

> Hi,
>
>> 
>> > On Thu, Jul 31, 2014 at 05:47:26PM +0800, arei.gonglei@huawei.com wrote:
>> > [...]
>> > > +void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
>> > > +                             const char *suffix)
>> > > +{
>> > > +    FWBootEntry *i, *old_entry = NULL;
>> > > +
>> > > +    assert(dev != NULL || suffix != NULL);
>> > > +
>> > > +    if (bootindex >= 0) {
>> > > +        QTAILQ_FOREACH(i, &fw_boot_order, link) {
>> > > +            if (i->bootindex == bootindex) {
>> > > +                qerror_report(ERROR_CLASS_GENERIC_ERROR,
>> > > +                              "The bootindex %d has already been
>> used",
>> > > +                              bootindex);
>> >
>> > Isn't an Error** parameter preferable here, instead of using
>> > qerror_report()?
>> 
>> Good catch. I'm not following this series, but using qerror_report() is
>> almost always wrong these days.

Yes.  http://wiki.qemu.org/CodeTransitions#Error_reporting explains:

    qerror_report() is a transitional interface to help with converting
    existing HMP commands to QMP. It should not be used elsewhere. Use
    Error objects instead with error_propagate() and error_setg()
    instead.

> Would you give me some advice? Thanks.
> Using error_report() instead of qerror_report()?

Depends on how the function is used.

If you know this can only run during QEMU startup (e.g. command line
processing) or in a *human* monitor, error_report() is fine.

If the error is propagated up the call chain to some place that reports
it via its Error ** parameter to its caller, then you should consider
passing an Error object created with error_setg() here up the call
chain.

Not the case right now, as your modify_boot_device_path() cannot fail.
Whether that's appropriate I can't tell without examining more of your
patch.

Re: [Qemu-devel] [PATCH v4 1/8] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

Hi,

> >>
> >> > On Thu, Jul 31, 2014 at 05:47:26PM +0800, arei.gonglei@huawei.com
> wrote:
> >> > [...]
> >> > > +void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
> >> > > +                             const char *suffix)
> >> > > +{
> >> > > +    FWBootEntry *i, *old_entry = NULL;
> >> > > +
> >> > > +    assert(dev != NULL || suffix != NULL);
> >> > > +
> >> > > +    if (bootindex >= 0) {
> >> > > +        QTAILQ_FOREACH(i, &fw_boot_order, link) {
> >> > > +            if (i->bootindex == bootindex) {
> >> > > +                qerror_report(ERROR_CLASS_GENERIC_ERROR,
> >> > > +                              "The bootindex %d has already
> been
> >> used",
> >> > > +                              bootindex);
> >> >
> >> > Isn't an Error** parameter preferable here, instead of using
> >> > qerror_report()?
> >>
> >> Good catch. I'm not following this series, but using qerror_report() is
> >> almost always wrong these days.
> 
> Yes.  http://wiki.qemu.org/CodeTransitions#Error_reporting explains:
> 
>     qerror_report() is a transitional interface to help with converting
>     existing HMP commands to QMP. It should not be used elsewhere. Use
>     Error objects instead with error_propagate() and error_setg()
>     instead.
> 
> > Would you give me some advice? Thanks.
> > Using error_report() instead of qerror_report()?
> 
> Depends on how the function is used.
> 
> If you know this can only run during QEMU startup (e.g. command line
> processing) or in a *human* monitor, error_report() is fine.
> 
> If the error is propagated up the call chain to some place that reports
> it via its Error ** parameter to its caller, then you should consider
> passing an Error object created with error_setg() here up the call
> chain.
> 
Understood, thank you so much!
error_setg() is the best choice in my case.

> Not the case right now, as your modify_boot_device_path() cannot fail.
> Whether that's appropriate I can't tell without examining more of your
> patch.

Best regards,
-Gonglei