[Qemu-devel] [PATCH v2 0/5] bootdevice: Refactor and improvement

[Qemu-devel] [PATCH v2 0/5] bootdevice: Refactor and improvement

[arei.gonglei]

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

Changes of v2:
 Thanks to Peter's report and suggestion.
 - fix 'make check' complaint.
 - fix inapposite using &error_abort in patch 5.

Because of Peter's review comments in a pull request, I start a new
round review about this patch series v2. Please review, Thanks.

Patch 1 just move boot order related code to bootdevice.c.
Patch 2,3,5 add an argument to corresponding functions.
This way, we can propagate the error messages to the caller.
Maybe somebody will say we will remove the legacy boot order
in the future, instead of using bootindex. But at present,
for PPC, the have no way support bootindex, ARM on the flight
(Laszlo Ersek) as far as know.

After this work, we can easily to add QMP command for existing
HMP command 'boot_set' if we have a requirement.


Gonglei (5):
  bootdevice: move code about bootorder from vl.c to bootdevice.c
  bootdevice: add Error **errp argument for validate_bootdevices()
  bootdevice: add Error **errp argument for qemu_boot_set()
  bootdevice: add validate check for qemu_boot_set()
  bootdevice: add Error **errp argument for QEMUBootSetHandler

 bootdevice.c            | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/pc.c            | 23 ++++++++-------
 hw/ppc/mac_newworld.c   |  4 +--
 hw/ppc/mac_oldworld.c   |  5 ++--
 hw/sparc/sun4m.c        |  4 +--
 hw/sparc64/sun4u.c      |  4 +--
 include/hw/hw.h         |  6 ----
 include/sysemu/sysemu.h |  8 +++++
 monitor.c               | 14 ++++-----
 vl.c                    | 77 +++++++++----------------------------------------
 10 files changed, 121 insertions(+), 97 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v2 1/5] bootdevice: move code about bootorder from vl.c to bootdevice.c

[arei.gonglei]

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

First, we can downsize vl.c, make it simpler by
little and little. Second, I can maintain those code
and make some improvement.

Cc: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 bootdevice.c            | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/hw.h         |  6 -----
 include/sysemu/sysemu.h |  8 +++++++
 vl.c                    | 62 -------------------------------------------------
 4 files changed, 70 insertions(+), 68 deletions(-)

diff --git a/bootdevice.c b/bootdevice.c
index b29970c..aae4cac 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -25,6 +25,7 @@
 #include "sysemu/sysemu.h"
 #include "qapi/visitor.h"
 #include "qemu/error-report.h"
+#include "hw/hw.h"
 
 typedef struct FWBootEntry FWBootEntry;
 
@@ -37,6 +38,67 @@ struct FWBootEntry {
 
 static QTAILQ_HEAD(, FWBootEntry) fw_boot_order =
     QTAILQ_HEAD_INITIALIZER(fw_boot_order);
+static QEMUBootSetHandler *boot_set_handler;
+static void *boot_set_opaque;
+
+void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
+{
+    boot_set_handler = func;
+    boot_set_opaque = opaque;
+}
+
+int qemu_boot_set(const char *boot_order)
+{
+    if (!boot_set_handler) {
+        return -EINVAL;
+    }
+    return boot_set_handler(boot_set_opaque, boot_order);
+}
+
+void validate_bootdevices(const char *devices)
+{
+    /* We just do some generic consistency checks */
+    const char *p;
+    int bitmap = 0;
+
+    for (p = devices; *p != '\0'; p++) {
+        /* Allowed boot devices are:
+         * a-b: floppy disk drives
+         * c-f: IDE disk drives
+         * g-m: machine implementation dependent drives
+         * n-p: network devices
+         * It's up to each machine implementation to check if the given boot
+         * devices match the actual hardware implementation and firmware
+         * features.
+         */
+        if (*p < 'a' || *p > 'p') {
+            fprintf(stderr, "Invalid boot device '%c'\n", *p);
+            exit(1);
+        }
+        if (bitmap & (1 << (*p - 'a'))) {
+            fprintf(stderr, "Boot device '%c' was given twice\n", *p);
+            exit(1);
+        }
+        bitmap |= 1 << (*p - 'a');
+    }
+}
+
+void restore_boot_order(void *opaque)
+{
+    char *normal_boot_order = opaque;
+    static int first = 1;
+
+    /* Restore boot order and remove ourselves after the first boot */
+    if (first) {
+        first = 0;
+        return;
+    }
+
+    qemu_boot_set(normal_boot_order);
+
+    qemu_unregister_reset(restore_boot_order, normal_boot_order);
+    g_free(normal_boot_order);
+}
 
 void check_boot_index(int32_t bootindex, Error **errp)
 {
diff --git a/include/hw/hw.h b/include/hw/hw.h
index 33bdb92..c78adae 100644
--- a/include/hw/hw.h
+++ b/include/hw/hw.h
@@ -41,12 +41,6 @@ typedef void QEMUResetHandler(void *opaque);
 void qemu_register_reset(QEMUResetHandler *func, void *opaque);
 void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
 
-/* handler to set the boot_device order for a specific type of QEMUMachine */
-/* return 0 if success */
-typedef int QEMUBootSetHandler(void *opaque, const char *boot_order);
-void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
-int qemu_boot_set(const char *boot_order);
-
 #ifdef NEED_CPU_H
 #if TARGET_LONG_BITS == 64
 #define VMSTATE_UINTTL_V(_f, _s, _v)                                  \
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 9fea3bc..84798ef 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -216,6 +216,14 @@ void del_boot_device_path(DeviceState *dev, const char *suffix);
 void device_add_bootindex_property(Object *obj, int32_t *bootindex,
                                    const char *name, const char *suffix,
                                    DeviceState *dev, Error **errp);
+void restore_boot_order(void *opaque);
+void validate_bootdevices(const char *devices);
+
+/* handler to set the boot_device order for a specific type of QEMUMachine */
+/* return 0 if success */
+typedef int QEMUBootSetHandler(void *opaque, const char *boot_order);
+void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
+int qemu_boot_set(const char *boot_order);
 
 QemuOpts *qemu_get_machine_opts(void);
 
diff --git a/vl.c b/vl.c
index 113e98e..f665621 100644
--- a/vl.c
+++ b/vl.c
@@ -196,9 +196,6 @@ NodeInfo numa_info[MAX_NODES];
 uint8_t qemu_uuid[16];
 bool qemu_uuid_set;
 
-static QEMUBootSetHandler *boot_set_handler;
-static void *boot_set_opaque;
-
 static NotifierList exit_notifiers =
     NOTIFIER_LIST_INITIALIZER(exit_notifiers);
 
@@ -1198,65 +1195,6 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
 
 }
 
-void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
-{
-    boot_set_handler = func;
-    boot_set_opaque = opaque;
-}
-
-int qemu_boot_set(const char *boot_order)
-{
-    if (!boot_set_handler) {
-        return -EINVAL;
-    }
-    return boot_set_handler(boot_set_opaque, boot_order);
-}
-
-static void validate_bootdevices(const char *devices)
-{
-    /* We just do some generic consistency checks */
-    const char *p;
-    int bitmap = 0;
-
-    for (p = devices; *p != '\0'; p++) {
-        /* Allowed boot devices are:
-         * a-b: floppy disk drives
-         * c-f: IDE disk drives
-         * g-m: machine implementation dependent drives
-         * n-p: network devices
-         * It's up to each machine implementation to check if the given boot
-         * devices match the actual hardware implementation and firmware
-         * features.
-         */
-        if (*p < 'a' || *p > 'p') {
-            fprintf(stderr, "Invalid boot device '%c'\n", *p);
-            exit(1);
-        }
-        if (bitmap & (1 << (*p - 'a'))) {
-            fprintf(stderr, "Boot device '%c' was given twice\n", *p);
-            exit(1);
-        }
-        bitmap |= 1 << (*p - 'a');
-    }
-}
-
-static void restore_boot_order(void *opaque)
-{
-    char *normal_boot_order = opaque;
-    static int first = 1;
-
-    /* Restore boot order and remove ourselves after the first boot */
-    if (first) {
-        first = 0;
-        return;
-    }
-
-    qemu_boot_set(normal_boot_order);
-
-    qemu_unregister_reset(restore_boot_order, normal_boot_order);
-    g_free(normal_boot_order);
-}
-
 static QemuOptsList qemu_smp_opts = {
     .name = "smp-opts",
     .implied_opt_name = "cpus",
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 2/5] bootdevice: add Error **errp argument for validate_bootdevices()

[arei.gonglei]

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

We can use it for checking when we change traditional
boot order dynamically and propagate error message
to the monitor.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 bootdevice.c            | 10 +++++-----
 include/sysemu/sysemu.h |  2 +-
 vl.c                    | 15 +++++++++++++--
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/bootdevice.c b/bootdevice.c
index aae4cac..184348e 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -55,7 +55,7 @@ int qemu_boot_set(const char *boot_order)
     return boot_set_handler(boot_set_opaque, boot_order);
 }
 
-void validate_bootdevices(const char *devices)
+void validate_bootdevices(const char *devices, Error **errp)
 {
     /* We just do some generic consistency checks */
     const char *p;
@@ -72,12 +72,12 @@ void validate_bootdevices(const char *devices)
          * features.
          */
         if (*p < 'a' || *p > 'p') {
-            fprintf(stderr, "Invalid boot device '%c'\n", *p);
-            exit(1);
+            error_setg(errp, "Invalid boot device '%c'", *p);
+            return;
         }
         if (bitmap & (1 << (*p - 'a'))) {
-            fprintf(stderr, "Boot device '%c' was given twice\n", *p);
-            exit(1);
+            error_setg(errp, "Boot device '%c' was given twice", *p);
+            return;
         }
         bitmap |= 1 << (*p - 'a');
     }
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 84798ef..1382d63 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -217,7 +217,7 @@ void device_add_bootindex_property(Object *obj, int32_t *bootindex,
                                    const char *name, const char *suffix,
                                    DeviceState *dev, Error **errp);
 void restore_boot_order(void *opaque);
-void validate_bootdevices(const char *devices);
+void validate_bootdevices(const char *devices, Error **errp);
 
 /* handler to set the boot_device order for a specific type of QEMUMachine */
 /* return 0 if success */
diff --git a/vl.c b/vl.c
index f665621..f0cdb63 100644
--- a/vl.c
+++ b/vl.c
@@ -4087,16 +4087,27 @@ int main(int argc, char **argv, char **envp)
     if (opts) {
         char *normal_boot_order;
         const char *order, *once;
+        Error *local_err = NULL;
 
         order = qemu_opt_get(opts, "order");
         if (order) {
-            validate_bootdevices(order);
+            validate_bootdevices(order, &local_err);
+            if (local_err) {
+                error_report("%s", error_get_pretty(local_err));
+                error_free(local_err);
+                exit(1);
+            }
             boot_order = order;
         }
 
         once = qemu_opt_get(opts, "once");
         if (once) {
-            validate_bootdevices(once);
+            validate_bootdevices(once, &local_err);
+            if (local_err) {
+                error_report("%s", error_get_pretty(local_err));
+                error_free(local_err);
+                exit(1);
+            }
             normal_boot_order = g_strdup(boot_order);
             boot_order = once;
             qemu_register_reset(restore_boot_order, normal_boot_order);
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 3/5] bootdevice: add Error **errp argument for qemu_boot_set()

[arei.gonglei]

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

We can use it for checking when we change traditional
boot order dynamically and propagate error message
to the monitor.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 bootdevice.c            | 14 ++++++++++----
 include/sysemu/sysemu.h |  2 +-
 monitor.c               | 14 ++++++--------
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/bootdevice.c b/bootdevice.c
index 184348e..7f07507 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -47,12 +47,18 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
     boot_set_opaque = opaque;
 }
 
-int qemu_boot_set(const char *boot_order)
+void qemu_boot_set(const char *boot_order, Error **errp)
 {
     if (!boot_set_handler) {
-        return -EINVAL;
+        error_setg(errp, "no function defined to set boot device list for"
+                         " this architecture");
+        return;
+    }
+
+    if (boot_set_handler(boot_set_opaque, boot_order)) {
+        error_setg(errp, "setting boot device list failed");
+        return;
     }
-    return boot_set_handler(boot_set_opaque, boot_order);
 }
 
 void validate_bootdevices(const char *devices, Error **errp)
@@ -94,7 +100,7 @@ void restore_boot_order(void *opaque)
         return;
     }
 
-    qemu_boot_set(normal_boot_order);
+    qemu_boot_set(normal_boot_order, NULL);
 
     qemu_unregister_reset(restore_boot_order, normal_boot_order);
     g_free(normal_boot_order);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 1382d63..ee7fee7 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -223,7 +223,7 @@ void validate_bootdevices(const char *devices, Error **errp);
 /* return 0 if success */
 typedef int QEMUBootSetHandler(void *opaque, const char *boot_order);
 void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
-int qemu_boot_set(const char *boot_order);
+void qemu_boot_set(const char *boot_order, Error **errp);
 
 QemuOpts *qemu_get_machine_opts(void);
 
diff --git a/monitor.c b/monitor.c
index b37ddda..62e21c5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1489,17 +1489,15 @@ static void do_ioport_write(Monitor *mon, const QDict *qdict)
 
 static void do_boot_set(Monitor *mon, const QDict *qdict)
 {
-    int res;
+    Error *local_err = NULL;
     const char *bootdevice = qdict_get_str(qdict, "bootdevice");
 
-    res = qemu_boot_set(bootdevice);
-    if (res == 0) {
-        monitor_printf(mon, "boot device list now set to %s\n", bootdevice);
-    } else if (res > 0) {
-        monitor_printf(mon, "setting boot device list failed\n");
+    qemu_boot_set(bootdevice, &local_err);
+    if (local_err) {
+        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
+        error_free(local_err);
     } else {
-        monitor_printf(mon, "no function defined to set boot device list for "
-                       "this architecture\n");
+        monitor_printf(mon, "boot device list now set to %s\n", bootdevice);
     }
 }
 
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 4/5] bootdevice: add validate check for qemu_boot_set()

[arei.gonglei]

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

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 bootdevice.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/bootdevice.c b/bootdevice.c
index 7f07507..9de34ba 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -49,12 +49,20 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
 
 void qemu_boot_set(const char *boot_order, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (!boot_set_handler) {
         error_setg(errp, "no function defined to set boot device list for"
                          " this architecture");
         return;
     }
 
+    validate_bootdevices(boot_order, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     if (boot_set_handler(boot_set_opaque, boot_order)) {
         error_setg(errp, "setting boot device list failed");
         return;
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 5/5] bootdevice: add Error **errp argument for QEMUBootSetHandler

[arei.gonglei]

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

We can use it for checking when we change traditional
boot order dynamically and propagate error message
to the monitor.
For x86 architecture, we pass &local_err to set_boot_dev()
when vm startup in pc_coms_init().

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 bootdevice.c            |  5 +----
 hw/i386/pc.c            | 23 +++++++++++++----------
 hw/ppc/mac_newworld.c   |  4 ++--
 hw/ppc/mac_oldworld.c   |  5 ++---
 hw/sparc/sun4m.c        |  4 ++--
 hw/sparc64/sun4u.c      |  4 ++--
 include/sysemu/sysemu.h |  4 ++--
 7 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/bootdevice.c b/bootdevice.c
index 9de34ba..5914417 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -63,10 +63,7 @@ void qemu_boot_set(const char *boot_order, Error **errp)
         return;
     }
 
-    if (boot_set_handler(boot_set_opaque, boot_order)) {
-        error_setg(errp, "setting boot device list failed");
-        return;
-    }
+    boot_set_handler(boot_set_opaque, boot_order, errp);
 }
 
 void validate_bootdevices(const char *devices, Error **errp)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c0e55a6..6b7fdea 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -282,7 +282,7 @@ static int boot_device2nibble(char boot_device)
     return 0;
 }
 
-static int set_boot_dev(ISADevice *s, const char *boot_device)
+static void set_boot_dev(ISADevice *s, const char *boot_device, Error **errp)
 {
 #define PC_MAX_BOOT_DEVICES 3
     int nbds, bds[3] = { 0, };
@@ -290,25 +290,24 @@ static int set_boot_dev(ISADevice *s, const char *boot_device)
 
     nbds = strlen(boot_device);
     if (nbds > PC_MAX_BOOT_DEVICES) {
-        error_report("Too many boot devices for PC");
-        return(1);
+        error_setg(errp, "Too many boot devices for PC");
+        return;
     }
     for (i = 0; i < nbds; i++) {
         bds[i] = boot_device2nibble(boot_device[i]);
         if (bds[i] == 0) {
-            error_report("Invalid boot device for PC: '%c'",
-                         boot_device[i]);
-            return(1);
+            error_setg(errp, "Invalid boot device for PC: '%c'",
+                       boot_device[i]);
+            return;
         }
     }
     rtc_set_memory(s, 0x3d, (bds[1] << 4) | bds[0]);
     rtc_set_memory(s, 0x38, (bds[2] << 4) | (fd_bootchk ? 0x0 : 0x1));
-    return(0);
 }
 
-static int pc_boot_set(void *opaque, const char *boot_device)
+static void pc_boot_set(void *opaque, const char *boot_device, Error **errp)
 {
-    return set_boot_dev(opaque, boot_device);
+    set_boot_dev(opaque, boot_device, errp);
 }
 
 typedef struct pc_cmos_init_late_arg {
@@ -365,6 +364,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
     FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
     static pc_cmos_init_late_arg arg;
     PCMachineState *pc_machine = PC_MACHINE(machine);
+    Error *local_err = NULL;
 
     /* various important CMOS locations needed by PC/Bochs bios */
 
@@ -412,7 +412,10 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
     object_property_set_link(OBJECT(machine), OBJECT(s),
                              "rtc_state", &error_abort);
 
-    if (set_boot_dev(s, boot_device)) {
+    set_boot_dev(s, boot_device, &local_err);
+    if (local_err) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
         exit(1);
     }
 
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 89aee71..ee1ed8a 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -116,10 +116,10 @@ static const MemoryRegionOps unin_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int fw_cfg_boot_set(void *opaque, const char *boot_device)
+static void fw_cfg_boot_set(void *opaque, const char *boot_device,
+                            Error **errp)
 {
     fw_cfg_add_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
-    return 0;
 }
 
 static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 32c21a4..15109c2 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -49,13 +49,12 @@
 #define CLOCKFREQ 266000000UL
 #define BUSFREQ 66000000UL
 
-static int fw_cfg_boot_set(void *opaque, const char *boot_device)
+static void fw_cfg_boot_set(void *opaque, const char *boot_device,
+                            Error **errp)
 {
     fw_cfg_add_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
-    return 0;
 }
 
-
 static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
 {
     return (addr & 0x0fffffff) + KERNEL_LOAD_ADDR;
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 8273199..df259ad 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -121,10 +121,10 @@ void DMA_register_channel (int nchan,
 {
 }
 
-static int fw_cfg_boot_set(void *opaque, const char *boot_device)
+static void fw_cfg_boot_set(void *opaque, const char *boot_device,
+                            Error **errp)
 {
     fw_cfg_add_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
-    return 0;
 }
 
 static void nvram_init(M48t59State *nvram, uint8_t *macaddr,
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index f42112c..acac8f9 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -124,10 +124,10 @@ void DMA_register_channel (int nchan,
 {
 }
 
-static int fw_cfg_boot_set(void *opaque, const char *boot_device)
+static void fw_cfg_boot_set(void *opaque, const char *boot_device,
+                            Error **errp)
 {
     fw_cfg_add_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
-    return 0;
 }
 
 static int sun4u_NVRAM_set_params(M48t59State *nvram, uint16_t NVRAM_size,
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index ee7fee7..503e5a4 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -220,8 +220,8 @@ void restore_boot_order(void *opaque);
 void validate_bootdevices(const char *devices, Error **errp);
 
 /* handler to set the boot_device order for a specific type of QEMUMachine */
-/* return 0 if success */
-typedef int QEMUBootSetHandler(void *opaque, const char *boot_order);
+typedef void QEMUBootSetHandler(void *opaque, const char *boot_order,
+                                Error **errp);
 void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
 void qemu_boot_set(const char *boot_order, Error **errp);
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2 1/5] bootdevice: move code about bootorder from vl.c to bootdevice.c

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> First, we can downsize vl.c, make it simpler by
> little and little. Second, I can maintain those code
> and make some improvement.

Any patch moving code from vl.c to a better place can't be all bad :)

[...]
> diff --git a/include/hw/hw.h b/include/hw/hw.h
> index 33bdb92..c78adae 100644
> --- a/include/hw/hw.h
> +++ b/include/hw/hw.h
> @@ -41,12 +41,6 @@ typedef void QEMUResetHandler(void *opaque);
>  void qemu_register_reset(QEMUResetHandler *func, void *opaque);
>  void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
>  
> -/* handler to set the boot_device order for a specific type of QEMUMachine */
> -/* return 0 if success */
> -typedef int QEMUBootSetHandler(void *opaque, const char *boot_order);
> -void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
> -int qemu_boot_set(const char *boot_order);
> -
>  #ifdef NEED_CPU_H
>  #if TARGET_LONG_BITS == 64
>  #define VMSTATE_UINTTL_V(_f, _s, _v)                                  \
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 9fea3bc..84798ef 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -216,6 +216,14 @@ void del_boot_device_path(DeviceState *dev, const char *suffix);
>  void device_add_bootindex_property(Object *obj, int32_t *bootindex,
>                                     const char *name, const char *suffix,
>                                     DeviceState *dev, Error **errp);
> +void restore_boot_order(void *opaque);
> +void validate_bootdevices(const char *devices);
> +
> +/* handler to set the boot_device order for a specific type of QEMUMachine */
> +/* return 0 if success */
> +typedef int QEMUBootSetHandler(void *opaque, const char *boot_order);
> +void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
> +int qemu_boot_set(const char *boot_order);
>  
>  QemuOpts *qemu_get_machine_opts(void);
>  

Any particular reason for moving the declarations from hw.h to sysemu.h?

[...]

Re: [Qemu-devel] [PATCH v2 2/5] bootdevice: add Error **errp argument for validate_bootdevices()

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> We can use it for checking when we change traditional
> boot order dynamically and propagate error message
> to the monitor.

In case you need to respin for some other reason: consider replacing "We
can use it" by something like "Will be useful", to make it clearer that
we can't change boot order dynamically, yet.

>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  bootdevice.c            | 10 +++++-----
>  include/sysemu/sysemu.h |  2 +-
>  vl.c                    | 15 +++++++++++++--
>  3 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/bootdevice.c b/bootdevice.c
> index aae4cac..184348e 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -55,7 +55,7 @@ int qemu_boot_set(const char *boot_order)
>      return boot_set_handler(boot_set_opaque, boot_order);
>  }
>  
> -void validate_bootdevices(const char *devices)
> +void validate_bootdevices(const char *devices, Error **errp)
>  {
>      /* We just do some generic consistency checks */
>      const char *p;
> @@ -72,12 +72,12 @@ void validate_bootdevices(const char *devices)
>           * features.
>           */
>          if (*p < 'a' || *p > 'p') {
> -            fprintf(stderr, "Invalid boot device '%c'\n", *p);
> -            exit(1);
> +            error_setg(errp, "Invalid boot device '%c'", *p);
> +            return;
>          }
>          if (bitmap & (1 << (*p - 'a'))) {
> -            fprintf(stderr, "Boot device '%c' was given twice\n", *p);
> -            exit(1);
> +            error_setg(errp, "Boot device '%c' was given twice", *p);
> +            return;
>          }
>          bitmap |= 1 << (*p - 'a');
>      }
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 84798ef..1382d63 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -217,7 +217,7 @@ void device_add_bootindex_property(Object *obj, int32_t *bootindex,
>                                     const char *name, const char *suffix,
>                                     DeviceState *dev, Error **errp);
>  void restore_boot_order(void *opaque);
> -void validate_bootdevices(const char *devices);
> +void validate_bootdevices(const char *devices, Error **errp);
>  
>  /* handler to set the boot_device order for a specific type of QEMUMachine */
>  /* return 0 if success */
> diff --git a/vl.c b/vl.c
> index f665621..f0cdb63 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4087,16 +4087,27 @@ int main(int argc, char **argv, char **envp)
>      if (opts) {
>          char *normal_boot_order;
>          const char *order, *once;
> +        Error *local_err = NULL;
>  
>          order = qemu_opt_get(opts, "order");
>          if (order) {
> -            validate_bootdevices(order);
> +            validate_bootdevices(order, &local_err);
> +            if (local_err) {
> +                error_report("%s", error_get_pretty(local_err));
> +                error_free(local_err);
> +                exit(1);
> +            }

I wouldn't bother freeing stuff right before exit().  But it's not
wrong.

>              boot_order = order;
>          }
>  
>          once = qemu_opt_get(opts, "once");
>          if (once) {
> -            validate_bootdevices(once);
> +            validate_bootdevices(once, &local_err);
> +            if (local_err) {
> +                error_report("%s", error_get_pretty(local_err));
> +                error_free(local_err);
> +                exit(1);
> +            }
>              normal_boot_order = g_strdup(boot_order);
>              boot_order = once;
>              qemu_register_reset(restore_boot_order, normal_boot_order);

Likewise.

Re: [Qemu-devel] [PATCH v2 3/5] bootdevice: add Error **errp argument for qemu_boot_set()

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> We can use it for checking when we change traditional
> boot order dynamically and propagate error message
> to the monitor.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>

I was about to write that this patch isn't an improvement, but then I
read PATCH 4, and now I understand what you're trying to accomplish.

Re: [Qemu-devel] [PATCH v2 5/5] bootdevice: add Error **errp argument for QEMUBootSetHandler

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> We can use it for checking when we change traditional
> boot order dynamically and propagate error message
> to the monitor.
> For x86 architecture, we pass &local_err to set_boot_dev()
> when vm startup in pc_coms_init().
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  bootdevice.c            |  5 +----
>  hw/i386/pc.c            | 23 +++++++++++++----------
>  hw/ppc/mac_newworld.c   |  4 ++--
>  hw/ppc/mac_oldworld.c   |  5 ++---
>  hw/sparc/sun4m.c        |  4 ++--
>  hw/sparc64/sun4u.c      |  4 ++--
>  include/sysemu/sysemu.h |  4 ++--
>  7 files changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/bootdevice.c b/bootdevice.c
> index 9de34ba..5914417 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -63,10 +63,7 @@ void qemu_boot_set(const char *boot_order, Error **errp)
>          return;
>      }
>  
> -    if (boot_set_handler(boot_set_opaque, boot_order)) {
> -        error_setg(errp, "setting boot device list failed");
> -        return;
> -    }
> +    boot_set_handler(boot_set_opaque, boot_order, errp);
>  }
>  
>  void validate_bootdevices(const char *devices, Error **errp)
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c0e55a6..6b7fdea 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -282,7 +282,7 @@ static int boot_device2nibble(char boot_device)
>      return 0;
>  }
>  
> -static int set_boot_dev(ISADevice *s, const char *boot_device)
> +static void set_boot_dev(ISADevice *s, const char *boot_device, Error **errp)
>  {
>  #define PC_MAX_BOOT_DEVICES 3
>      int nbds, bds[3] = { 0, };
> @@ -290,25 +290,24 @@ static int set_boot_dev(ISADevice *s, const char *boot_device)
>  
>      nbds = strlen(boot_device);
>      if (nbds > PC_MAX_BOOT_DEVICES) {
> -        error_report("Too many boot devices for PC");
> -        return(1);
> +        error_setg(errp, "Too many boot devices for PC");
> +        return;
>      }
>      for (i = 0; i < nbds; i++) {
>          bds[i] = boot_device2nibble(boot_device[i]);
>          if (bds[i] == 0) {
> -            error_report("Invalid boot device for PC: '%c'",
> -                         boot_device[i]);
> -            return(1);
> +            error_setg(errp, "Invalid boot device for PC: '%c'",
> +                       boot_device[i]);
> +            return;
>          }
>      }
>      rtc_set_memory(s, 0x3d, (bds[1] << 4) | bds[0]);
>      rtc_set_memory(s, 0x38, (bds[2] << 4) | (fd_bootchk ? 0x0 : 0x1));
> -    return(0);
>  }
>  
> -static int pc_boot_set(void *opaque, const char *boot_device)
> +static void pc_boot_set(void *opaque, const char *boot_device, Error **errp)
>  {
> -    return set_boot_dev(opaque, boot_device);
> +    set_boot_dev(opaque, boot_device, errp);
>  }
>  
>  typedef struct pc_cmos_init_late_arg {
> @@ -365,6 +364,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>      FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>      static pc_cmos_init_late_arg arg;
>      PCMachineState *pc_machine = PC_MACHINE(machine);
> +    Error *local_err = NULL;
>  
>      /* various important CMOS locations needed by PC/Bochs bios */
>  
> @@ -412,7 +412,10 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>      object_property_set_link(OBJECT(machine), OBJECT(s),
>                               "rtc_state", &error_abort);
>  
> -    if (set_boot_dev(s, boot_device)) {
> +    set_boot_dev(s, boot_device, &local_err);
> +    if (local_err) {
> +        error_report("%s", error_get_pretty(local_err));
> +        error_free(local_err);
>          exit(1);
>      }
>  

I wouldn't bother freeing stuff right before exit().  But it's not
wrong.

[...]

Re: [Qemu-devel] [PATCH v2 0/5] bootdevice: Refactor and improvement

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> Changes of v2:
>  Thanks to Peter's report and suggestion.
>  - fix 'make check' complaint.
>  - fix inapposite using &error_abort in patch 5.
>
> Because of Peter's review comments in a pull request, I start a new
> round review about this patch series v2. Please review, Thanks.
>
> Patch 1 just move boot order related code to bootdevice.c.
> Patch 2,3,5 add an argument to corresponding functions.
> This way, we can propagate the error messages to the caller.
> Maybe somebody will say we will remove the legacy boot order
> in the future, instead of using bootindex. But at present,
> for PPC, the have no way support bootindex, ARM on the flight
> (Laszlo Ersek) as far as know.
>
> After this work, we can easily to add QMP command for existing
> HMP command 'boot_set' if we have a requirement.

I don't understand why you move declarations to sysemu.h, and I had a
few remarks here and there.  But I couldn't see anything wrong, so:

Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/5] bootdevice: Refactor and improvement

[Gonglei]

On 2014/12/20 1:27, Markus Armbruster wrote:

> I don't understand why you move declarations to sysemu.h, and I had a
> few remarks here and there.  But I couldn't see anything wrong, so:
> 

My thought is just moving the code related boot order to a unified place. :(

> Series
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks for your review.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 2/5] bootdevice: add Error **errp argument for validate_bootdevices()

[Gonglei]

On 2014/12/20 1:17, Markus Armbruster wrote:

> <arei.gonglei@huawei.com> writes:
> 
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> We can use it for checking when we change traditional
>> boot order dynamically and propagate error message
>> to the monitor.
> 
> In case you need to respin for some other reason: consider replacing "We
> can use it" by something like "Will be useful", to make it clearer that
> we can't change boot order dynamically, yet.
> 

Actually, we had HMP command to change boot order dynamically at present.

>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  bootdevice.c            | 10 +++++-----
>>  include/sysemu/sysemu.h |  2 +-
>>  vl.c                    | 15 +++++++++++++--
>>  3 files changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/bootdevice.c b/bootdevice.c
>> index aae4cac..184348e 100644
>> --- a/bootdevice.c
>> +++ b/bootdevice.c
>> @@ -55,7 +55,7 @@ int qemu_boot_set(const char *boot_order)
>>      return boot_set_handler(boot_set_opaque, boot_order);
>>  }
>>  
>> -void validate_bootdevices(const char *devices)
>> +void validate_bootdevices(const char *devices, Error **errp)
>>  {
>>      /* We just do some generic consistency checks */
>>      const char *p;
>> @@ -72,12 +72,12 @@ void validate_bootdevices(const char *devices)
>>           * features.
>>           */
>>          if (*p < 'a' || *p > 'p') {
>> -            fprintf(stderr, "Invalid boot device '%c'\n", *p);
>> -            exit(1);
>> +            error_setg(errp, "Invalid boot device '%c'", *p);
>> +            return;
>>          }
>>          if (bitmap & (1 << (*p - 'a'))) {
>> -            fprintf(stderr, "Boot device '%c' was given twice\n", *p);
>> -            exit(1);
>> +            error_setg(errp, "Boot device '%c' was given twice", *p);
>> +            return;
>>          }
>>          bitmap |= 1 << (*p - 'a');
>>      }
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index 84798ef..1382d63 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -217,7 +217,7 @@ void device_add_bootindex_property(Object *obj, int32_t *bootindex,
>>                                     const char *name, const char *suffix,
>>                                     DeviceState *dev, Error **errp);
>>  void restore_boot_order(void *opaque);
>> -void validate_bootdevices(const char *devices);
>> +void validate_bootdevices(const char *devices, Error **errp);
>>  
>>  /* handler to set the boot_device order for a specific type of QEMUMachine */
>>  /* return 0 if success */
>> diff --git a/vl.c b/vl.c
>> index f665621..f0cdb63 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4087,16 +4087,27 @@ int main(int argc, char **argv, char **envp)
>>      if (opts) {
>>          char *normal_boot_order;
>>          const char *order, *once;
>> +        Error *local_err = NULL;
>>  
>>          order = qemu_opt_get(opts, "order");
>>          if (order) {
>> -            validate_bootdevices(order);
>> +            validate_bootdevices(order, &local_err);
>> +            if (local_err) {
>> +                error_report("%s", error_get_pretty(local_err));
>> +                error_free(local_err);
>> +                exit(1);
>> +            }
> 
> I wouldn't bother freeing stuff right before exit().  But it's not
> wrong.
> 

Ok, I can remove error_free(local_err) in spite of some other places
also doing it like this in vl.c. :)

>>              boot_order = order;
>>          }
>>  
>>          once = qemu_opt_get(opts, "once");
>>          if (once) {
>> -            validate_bootdevices(once);
>> +            validate_bootdevices(once, &local_err);
>> +            if (local_err) {
>> +                error_report("%s", error_get_pretty(local_err));
>> +                error_free(local_err);
>> +                exit(1);
>> +            }
>>              normal_boot_order = g_strdup(boot_order);
>>              boot_order = once;
>>              qemu_register_reset(restore_boot_order, normal_boot_order);
> 
> Likewise.

OK.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 5/5] bootdevice: add Error **errp argument for QEMUBootSetHandler

[Gonglei]

On 2014/12/20 1:24, Markus Armbruster wrote:

> <arei.gonglei@huawei.com> writes:
> 
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> We can use it for checking when we change traditional
>> boot order dynamically and propagate error message
>> to the monitor.
>> For x86 architecture, we pass &local_err to set_boot_dev()
>> when vm startup in pc_coms_init().
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Alexander Graf <agraf@suse.de>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> Cc: qemu-ppc@nongnu.org
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  bootdevice.c            |  5 +----
>>  hw/i386/pc.c            | 23 +++++++++++++----------
>>  hw/ppc/mac_newworld.c   |  4 ++--
>>  hw/ppc/mac_oldworld.c   |  5 ++---
>>  hw/sparc/sun4m.c        |  4 ++--
>>  hw/sparc64/sun4u.c      |  4 ++--
>>  include/sysemu/sysemu.h |  4 ++--
>>  7 files changed, 24 insertions(+), 25 deletions(-)
>>
>> diff --git a/bootdevice.c b/bootdevice.c
>> index 9de34ba..5914417 100644
>> --- a/bootdevice.c
>> +++ b/bootdevice.c
>> @@ -63,10 +63,7 @@ void qemu_boot_set(const char *boot_order, Error **errp)
>>          return;
>>      }
>>  
>> -    if (boot_set_handler(boot_set_opaque, boot_order)) {
>> -        error_setg(errp, "setting boot device list failed");
>> -        return;
>> -    }
>> +    boot_set_handler(boot_set_opaque, boot_order, errp);
>>  }
>>  
>>  void validate_bootdevices(const char *devices, Error **errp)
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index c0e55a6..6b7fdea 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -282,7 +282,7 @@ static int boot_device2nibble(char boot_device)
>>      return 0;
>>  }
>>  
>> -static int set_boot_dev(ISADevice *s, const char *boot_device)
>> +static void set_boot_dev(ISADevice *s, const char *boot_device, Error **errp)
>>  {
>>  #define PC_MAX_BOOT_DEVICES 3
>>      int nbds, bds[3] = { 0, };
>> @@ -290,25 +290,24 @@ static int set_boot_dev(ISADevice *s, const char *boot_device)
>>  
>>      nbds = strlen(boot_device);
>>      if (nbds > PC_MAX_BOOT_DEVICES) {
>> -        error_report("Too many boot devices for PC");
>> -        return(1);
>> +        error_setg(errp, "Too many boot devices for PC");
>> +        return;
>>      }
>>      for (i = 0; i < nbds; i++) {
>>          bds[i] = boot_device2nibble(boot_device[i]);
>>          if (bds[i] == 0) {
>> -            error_report("Invalid boot device for PC: '%c'",
>> -                         boot_device[i]);
>> -            return(1);
>> +            error_setg(errp, "Invalid boot device for PC: '%c'",
>> +                       boot_device[i]);
>> +            return;
>>          }
>>      }
>>      rtc_set_memory(s, 0x3d, (bds[1] << 4) | bds[0]);
>>      rtc_set_memory(s, 0x38, (bds[2] << 4) | (fd_bootchk ? 0x0 : 0x1));
>> -    return(0);
>>  }
>>  
>> -static int pc_boot_set(void *opaque, const char *boot_device)
>> +static void pc_boot_set(void *opaque, const char *boot_device, Error **errp)
>>  {
>> -    return set_boot_dev(opaque, boot_device);
>> +    set_boot_dev(opaque, boot_device, errp);
>>  }
>>  
>>  typedef struct pc_cmos_init_late_arg {
>> @@ -365,6 +364,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>      FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>>      static pc_cmos_init_late_arg arg;
>>      PCMachineState *pc_machine = PC_MACHINE(machine);
>> +    Error *local_err = NULL;
>>  
>>      /* various important CMOS locations needed by PC/Bochs bios */
>>  
>> @@ -412,7 +412,10 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>      object_property_set_link(OBJECT(machine), OBJECT(s),
>>                               "rtc_state", &error_abort);
>>  
>> -    if (set_boot_dev(s, boot_device)) {
>> +    set_boot_dev(s, boot_device, &local_err);
>> +    if (local_err) {
>> +        error_report("%s", error_get_pretty(local_err));
>> +        error_free(local_err);
>>          exit(1);
>>      }
>>  
> 
> I wouldn't bother freeing stuff right before exit().  But it's not
> wrong.
> 

Will remove it, thanks.

Regards,
-Gonglei