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

[Qemu-devel] [PATCH v3 0/7] 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 -monitor stdio
QEMU 2.0.93 monitor - type 'help' for more information
(qemu) set-bootindex ide0-0-1 1
The bootindex 1 has already been used
(qemu) set-bootindex ide0-0-1 5 "/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 nic2 0
The bootindex 0 has already been used
(qemu) set-bootindex ide0-0-1 -1
(qemu) set-bootindex nic2 0
(qemu) system_reset
(qemu)

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.

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.

Gonglei (7):
  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
  spapr: fix possible memory leak

 hmp-commands.hx           | 15 ++++++++++
 hmp.c                     | 13 ++++++++
 hmp.h                     |  1 +
 hw/core/qdev.c            |  2 ++
 hw/nvram/fw_cfg.c         | 54 +++++++++++++++++++++++++++++-----
 hw/ppc/spapr.c            |  1 +
 include/hw/nvram/fw_cfg.h |  2 ++
 include/sysemu/sysemu.h   |  3 ++
 qapi-schema.json          | 16 ++++++++++
 qmp-commands.hx           | 24 +++++++++++++++
 qmp.c                     | 17 +++++++++++
 vl.c                      | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 12 files changed, 216 insertions(+), 7 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v3 1/7] 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.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 include/sysemu/sysemu.h |  2 ++
 vl.c                    | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 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..3e42eaa 100644
--- a/vl.c
+++ b/vl.c
@@ -1248,6 +1248,64 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
     QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
 }
 
+void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
+                          const char *suffix)
+{
+    FWBootEntry *node, *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 (i->dev->id && !strcmp(i->dev->id, dev->id)) {
+            QTAILQ_REMOVE(&fw_boot_order, i, link);
+            old_entry = i;
+
+            break;
+        }
+    }
+
+    if (bootindex >= 0) {
+        node = g_malloc0(sizeof(FWBootEntry));
+        node->bootindex = bootindex;
+        if (suffix) {
+            node->suffix = g_strdup(suffix);
+        } else if (old_entry) {
+            node->suffix = g_strdup(old_entry->suffix);
+        } else {
+            node->suffix = NULL;
+        }
+        node->dev = dev;
+
+        /* add to the global boot list */
+        QTAILQ_FOREACH(i, &fw_boot_order, link) {
+            if (i->bootindex < bootindex) {
+                continue;
+            }
+            QTAILQ_INSERT_BEFORE(i, node, link);
+            goto out;
+        }
+
+        QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
+    }
+
+out:
+    if (old_entry)  {
+        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 v3 2/7] bootindex: add del_boot_device_path function

[arei.gonglei]

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

Introduce a del_boot_device_path() cleanup fw_cfg content
when hot-unplugging devcie refer to bootindex.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 include/sysemu/sysemu.h |  1 +
 vl.c                    | 17 +++++++++++++++++
 2 files changed, 18 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 3e42eaa..0cdadb4 100644
--- a/vl.c
+++ b/vl.c
@@ -1248,6 +1248,23 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
     QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
 }
 
+void del_boot_device_path(DeviceState *dev)
+{
+    FWBootEntry *i;
+
+    assert(dev != NULL);
+
+    QTAILQ_FOREACH(i, &fw_boot_order, link) {
+        if (i->dev->id && dev->id && !strcmp(i->dev->id, dev->id)) {
+            /* remove all entries of the assigend 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 v3 3/7] 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 v3 4/7] 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/core/qdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index da1ba48..7bc12bc 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -975,6 +975,8 @@ static void device_finalize(Object *obj)
     if (dev->opts) {
         qemu_opts_del(dev->opts);
     }
+    /* remove device's bootindex from global boot order list */
+    del_boot_device_path(dev);
 
     QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
         QLIST_REMOVE(ngl, node);
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 5/7] 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 | 16 ++++++++++++++++
 qmp-commands.hx  | 24 ++++++++++++++++++++++++
 qmp.c            | 17 +++++++++++++++++
 3 files changed, 57 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index b11aad2..a9ef0be 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1704,6 +1704,22 @@
 { 'command': 'device_del', 'data': {'id': 'str'} }
 
 ##
+# @set-bootindex:
+#
+# set bootindex of a devcie
+#
+# @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..2c89a97 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -330,6 +330,30 @@ 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..f2c3c14 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 (NULL == 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 v3 6/7] 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 v3 7/7] spapr: fix possible memory leak

[arei.gonglei]

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

get_boot_devices_list() will malloc memory, spapr_finalize_fdt
doesn't free it.

Signed-off-by: Chenliang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/ppc/spapr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d01978f..edff5ce 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -745,6 +745,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
 
     cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
 
+    g_free(bootlist);
     g_free(fdt);
 }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 1/7] bootindex: add modify_boot_device_path function

[陈梁]

Hi
> +    if (bootindex >= 0) {
> +        node = g_malloc0(sizeof(FWBootEntry));
> +        node->bootindex = bootindex;
> +        if (suffix) {
> +            node->suffix = g_strdup(suffix);
> +        } else if (old_entry) {
> +            node->suffix = g_strdup(old_entry->suffix);
> +        } else {
> +            node->suffix = NULL;
> +        }
> +        node->dev = dev;
> +
> +        /* add to the global boot list */
> +        QTAILQ_FOREACH(i, &fw_boot_order, link) {
> +            if (i->bootindex < bootindex) {
> +                continue;
> +            }
> +            QTAILQ_INSERT_BEFORE(i, node, link);
> +            goto out;
> +        }
> +
> +        QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
> +    }

this code can be simply like this:

suffix = suffix ? suffix : old_entry->suffix ? old_entry->suffix : NULL;

add_boot_device_path(boot_index, dev, suffix) 

Best regards
Chen Liang

Re: [Qemu-devel] [PATCH v3 1/7] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

> -----Original Message-----
> From: 陈梁 [mailto:chenliang0016@icloud.com]
> Sent: Sunday, July 27, 2014 11:51 AM
> Subject: Re: [Qemu-devel] [PATCH v3 1/7] bootindex: add
> modify_boot_device_path function
> 
> Hi
> > +    if (bootindex >= 0) {
> > +        node = g_malloc0(sizeof(FWBootEntry));
> > +        node->bootindex = bootindex;
> > +        if (suffix) {
> > +            node->suffix = g_strdup(suffix);
> > +        } else if (old_entry) {
> > +            node->suffix = g_strdup(old_entry->suffix);
> > +        } else {
> > +            node->suffix = NULL;
> > +        }
> > +        node->dev = dev;
> > +
> > +        /* add to the global boot list */
> > +        QTAILQ_FOREACH(i, &fw_boot_order, link) {
> > +            if (i->bootindex < bootindex) {
> > +                continue;
> > +            }
> > +            QTAILQ_INSERT_BEFORE(i, node, link);
> > +            goto out;
> > +        }
> > +
> > +        QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
> > +    }
> 
> this code can be simply like this:
> 
> suffix = suffix ? suffix : old_entry->suffix ? old_entry->suffix : NULL;
> 
> add_boot_device_path(boot_index, dev, suffix)
> 
Nice, thanks.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v3 5/7] qmp: add set-bootindex command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

On 07/25/2014 10:45 PM, arei.gonglei@huawei.com wrote:
> 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 | 16 ++++++++++++++++
>  qmp-commands.hx  | 24 ++++++++++++++++++++++++
>  qmp.c            | 17 +++++++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b11aad2..a9ef0be 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1704,6 +1704,22 @@
>  { 'command': 'device_del', 'data': {'id': 'str'} }
>  
>  ##
> +# @set-bootindex:
> +#
> +# set bootindex of a devcie

s/devcie/device/

Just to make sure this command is not write-only, it would be nice to
mention which query-* command can be used to learn a device's current
bootindex.

> +#
> +# @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'} }

Long line; wrap it to stay in 80 columns.


> +
> +SQMP
> +set-bootindex
> +--------------------

Match the ---- length to the command name.

> +++ 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)

Indentation is off.

> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_find_recursive(sysbus_get_default(), id);
> +    if (NULL == dev) {

Code like Yoda we do not.  This is more idiomatically written 'if
(!dev)' or 'if (dev == NULL)'.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v3 5/7] qmp: add set-bootindex command

[Gonglei (Arei)]

Hi,

> -----Original Message-----
> From: Eric Blake [mailto:eblake@redhat.com]
> Sent: Thursday, July 31, 2014 6:37 AM
> Subject: Re: [PATCH v3 5/7] qmp: add set-bootindex command
> 
> On 07/25/2014 10:45 PM, arei.gonglei@huawei.com wrote:
> > 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 | 16 ++++++++++++++++
> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
> >  qmp.c            | 17 +++++++++++++++++
> >  3 files changed, 57 insertions(+)
> >
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index b11aad2..a9ef0be 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1704,6 +1704,22 @@
> >  { 'command': 'device_del', 'data': {'id': 'str'} }
> >
> >  ##
> > +# @set-bootindex:
> > +#
> > +# set bootindex of a devcie
> 
> s/devcie/device/
> 
OK.

> Just to make sure this command is not write-only, it would be nice to
> mention which query-* command can be used to learn a device's current
> bootindex.
> 
Yes. I am thinking about introducing a query-bootindex command, which
can show a device's current bootindex and bootindex's suffix.

> > +#
> > +# @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'} }
> 
> Long line; wrap it to stay in 80 columns.
> 
OK.

> 
> > +
> > +SQMP
> > +set-bootindex
> > +--------------------
> 
> Match the ---- length to the command name.
> 
OK.

> > +++ 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)
> 
> Indentation is off.
> 
OK.

> > +{
> > +    DeviceState *dev;
> > +
> > +    dev = qdev_find_recursive(sysbus_get_default(), id);
> > +    if (NULL == dev) {
> 
> Code like Yoda we do not.  This is more idiomatically written 'if
> (!dev)' or 'if (dev == NULL)'.
> 
OK.

Thanks for your careful reviewing, Eric. 
I will fix those problems in the next version.

> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v3 2/7] bootindex: add del_boot_device_path function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On 07/25/2014 10:45 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Introduce a del_boot_device_path() cleanup fw_cfg content
> when hot-unplugging devcie refer to bootindex.

s/devcie/device/

sounds odd; maybe:

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

> +
> +    QTAILQ_FOREACH(i, &fw_boot_order, link) {
> +        if (i->dev->id && dev->id && !strcmp(i->dev->id, dev->id)) {
> +            /* remove all entries of the assigend dev */

s/assigend/assigned/


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v3 2/7] bootindex: add del_boot_device_path function

[Gonglei (Arei)]

Hi,

> > Introduce a del_boot_device_path() cleanup fw_cfg content
> > when hot-unplugging devcie refer to bootindex.
> 
> s/devcie/device/
> 
> sounds odd; maybe:
> 
> Introduce del_boot_device_path() to clean up fw_cfg content when
> hot-unplugging a device that refers to a bootindex.
> 
OK. Thanks, Eric.

> > +
> > +    QTAILQ_FOREACH(i, &fw_boot_order, link) {
> > +        if (i->dev->id && dev->id && !strcmp(i->dev->id, dev->id)) {
> > +            /* remove all entries of the assigend dev */
> 
> s/assigend/assigned/
> 
OK.

> 
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v3 2/7] bootindex: add del_boot_device_path function

[Eduardo Habkost]

On Sat, Jul 26, 2014 at 12:45:28PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Introduce a del_boot_device_path() cleanup fw_cfg content
> when hot-unplugging devcie refer to bootindex.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Chenliang <chenliang88@huawei.com>
> ---
>  include/sysemu/sysemu.h |  1 +
>  vl.c                    | 17 +++++++++++++++++
>  2 files changed, 18 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 3e42eaa..0cdadb4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1248,6 +1248,23 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>      QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
>  }
>  
> +void del_boot_device_path(DeviceState *dev)
> +{
> +    FWBootEntry *i;
> +
> +    assert(dev != NULL);
> +
> +    QTAILQ_FOREACH(i, &fw_boot_order, link) {
> +        if (i->dev->id && dev->id && !strcmp(i->dev->id, dev->id)) {

What if the device doesn't have any ID set? I don't see anything on
add_boot_device_path() ensuring that dev->id is always set.

Why you don't just check if i->dev == dev?


> +            /* remove all entries of the assigend 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
> 
> 
> 

-- 
Eduardo