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 <l...@dorileo.org> > Signed-off-by: Chunyan Liu <cy...@suse.com> > --- > 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 <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
