[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v25 04/31] QemuOpts: change opt->name|str from (
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v25 04/31] QemuOpts: change opt->name|str from (const char *) to (char *) |
Date: |
Mon, 21 Apr 2014 12:53:00 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 |
On 04/10/2014 11:54 AM, Chunyan Liu wrote:
> qemu_opt_del() already assumes that all QemuOpt instances contain
> malloc'd name and value; but it had to cast away const because
> opts_start_struct() was doing its own thing and using static storage
> instead. By using the correct type and malloced strings everywhere, the
> usage of this struct becomes clearer.
>
> Reviewed-by: Leandro Dorileo <address@hidden>
> Signed-off-by: Chunyan Liu <address@hidden>
> ---
> include/qemu/option_int.h | 4 ++--
> qapi/opts-visitor.c | 10 +++++++---
> util/qemu-option.c | 4 ++--
> 3 files changed, 11 insertions(+), 7 deletions(-)
> @@ -177,7 +177,11 @@ opts_end_struct(Visitor *v, Error **errp)
> }
> g_hash_table_destroy(ov->unprocessed_opts);
> ov->unprocessed_opts = NULL;
> - g_free(ov->fake_id_opt);
> + if (ov->fake_id_opt) {
> + g_free(ov->fake_id_opt->name);
> + g_free(ov->fake_id_opt->str);
> + g_free(ov->fake_id_opt);
> + }
I wonder if this could have called qemu_opt_del() instead of open-coding
the three g_free() calls. On the other hand...
> ov->fake_id_opt = NULL;
> }
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 84f0d5c..065ffb8 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -664,8 +664,8 @@ static void qemu_opt_parse(QemuOpt *opt, Error **errp)
> static void qemu_opt_del(QemuOpt *opt)
> {
> QTAILQ_REMOVE(&opt->opts->head, opt, next);
...I don't know if the additional QTAILQ_REMOVE would be safe on
fake_id_opt, and you'd have to make qemu_opt_del non-static to call it
from another file.
> - g_free((/* !const */ char*)opt->name);
> - g_free((/* !const */ char*)opt->str);
> + g_free(opt->name);
> + g_free(opt->str);
> g_free(opt);
> }
So I'm fine with leaving this patch as-is:
Reviewed-by: Eric Blake <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v25 00/31] replace QEMUOptionParameter with QemuOpts, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 02/31] QemuOpts: add def_value_str to QemuOptDesc, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 05/31] QemuOpts: move qemu_opt_del ahead for later calling, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 04/31] QemuOpts: change opt->name|str from (const char *) to (char *), Chunyan Liu, 2014/04/11
- Re: [Qemu-devel] [PATCH v25 04/31] QemuOpts: change opt->name|str from (const char *) to (char *),
Eric Blake <=
- [Qemu-devel] [PATCH v25 01/31] QemuOpts: move find_desc_by_name ahead for later calling, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 03/31] qapi: output def_value_str when query command line options, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 06/31] QemuOpts: add qemu_opt_get_*_del functions for replace work, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 07/31] QemuOpts: add qemu_opts_print_help to replace print_option_help, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 09/31] QemuOpts: add qemu_opts_append to replace append_option_parameters, Chunyan Liu, 2014/04/11