[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 07/27] vl.c: add setter/getter functions for
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH v6 07/27] vl.c: add setter/getter functions for bootindex property |
Date: |
Thu, 4 Sep 2014 12:01:11 -0300 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Sat, Aug 30, 2014 at 06:00:07PM +0800, address@hidden wrote:
> From: Gonglei <address@hidden>
>
> when we remove bootindex form qdev.property to qom.property,
> we can use those functions set/get bootindex property for all
> correlative devices.
>
> Signed-off-by: Gonglei <address@hidden>
> ---
> include/sysemu/sysemu.h | 4 ++++
> vl.c | 27 +++++++++++++++++++++++++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 672984c..ca231e4 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -208,6 +208,10 @@ void do_usb_del(Monitor *mon, const QDict *qdict);
> void usb_info(Monitor *mon, const QDict *qdict);
>
> void check_boot_index(int32_t bootindex, Error **errp);
> +void get_bootindex(int32_t *bootindex, Visitor *v,
> + const char *name, Error **errp);
> +void set_bootindex(int32_t *bootindex, Visitor *v,
> + const char *name, Error **errp);
> void del_boot_device_path(DeviceState *dev);
> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> const char *suffix);
> diff --git a/vl.c b/vl.c
> index f2c3b2d..4363185 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1252,6 +1252,33 @@ void check_boot_index(int32_t bootindex, Error **errp)
> }
> }
>
> +void get_bootindex(int32_t *bootindex, Visitor *v,
> + const char *name, Error **errp)
> +{
> + visit_type_int32(v, bootindex, name, errp);
> +}
> +
> +void set_bootindex(int32_t *bootindex, Visitor *v,
> + const char *name, Error **errp)
> +{
> + int32_t boot_index;
> + Error *local_err = NULL;
> +
> + visit_type_int32(v, &boot_index, name, &local_err);
> +
> + if (local_err == NULL) {
> + /* check the bootindex existes or not in fw_boot_order list */
> + check_boot_index(boot_index, &local_err);
> + }
> +
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + /* change bootindex to a new one */
> + *bootindex = boot_index;
I believe the following pattern is more usual (and I find it easier to
read):
visit_type_int32(v, &boot_index, name, &local_err);
if (local_err) {
goto out;
}
check_boot_index(boot_index, &local_err);
if (local_err) {
goto out;
}
*bootindex = boot_index;
out:
if (local_err) {
error_propagate(errp, local_err);
}
Also, this function (the property setter) is where I expected
add_boot_device_path() to be called, instead of requiring every device
to add a reset handler and call add_boot_device_path() manually.
I suggest creating a new file for all the bootindex/boot-device code
(maybe call it bootdevice.c?), instead of adding more code to vl.c.
> +}
> +
> static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst)
> {
> bool ret = false;
> --
> 1.7.12.4
>
>
--
Eduardo