| Message ID | 1395396763-26081-5-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 21, 2014 at 06:12:15PM +0800, Chunyan Liu wrote: > Signed-off-by: Chunyan Liu <cyliu@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(-) > > diff --git a/include/qemu/option_int.h b/include/qemu/option_int.h > index 8212fa4..db9ed91 100644 > --- a/include/qemu/option_int.h > +++ b/include/qemu/option_int.h > @@ -30,8 +30,8 @@ > #include "qemu/error-report.h" > > struct QemuOpt { > - const char *name; > - const char *str; > + char *name; > + char *str; I still don't see why you need this change. I understand you're strdup'ing name and str, what I can't get is why you need this, and since the lack of description in the patch itself I don't know what you're fixing here. I saw the opts-visitor.c users and they're mostly controlling their QemuOpts instances, their instances come after a qemu_opts_from_qdict() or qemu_opts_parse(). I haven't seen the users with too much inversion but I guess they follow the same pattern. I may be missing something completely obvious here but if you really need this change could you introduce a test in the test-opts-visitor.c test suite so we can understand the problem you're fixing here? Regards...
On 03/21/2014 04:12 AM, Chunyan Liu wrote: Your subject says "what", but your commit message lacks a "why". Without a good reason, it's hard to see what this patch is good for. May I suggest: 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. > Signed-off-by: Chunyan Liu <cyliu@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(-) > > diff --git a/include/qemu/option_int.h b/include/qemu/option_int.h > index 8212fa4..db9ed91 100644 > --- a/include/qemu/option_int.h > +++ b/include/qemu/option_int.h > @@ -30,8 +30,8 @@ > #include "qemu/error-report.h" > > struct QemuOpt { > - const char *name; > - const char *str; > + char *name; > + char *str; While touching this, is it worth trimming the extra spacing before '*'? It's not like you are preserving alignment or anything. But as that's cosmetic, and as the code itself is correct, I'm fine if you add this when you expand your commit message: Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/include/qemu/option_int.h b/include/qemu/option_int.h index 8212fa4..db9ed91 100644 --- a/include/qemu/option_int.h +++ b/include/qemu/option_int.h @@ -30,8 +30,8 @@ #include "qemu/error-report.h" struct QemuOpt { - const char *name; - const char *str; + char *name; + char *str; const QemuOptDesc *desc; union { diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index 5d830a2..7a64f4e 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -143,8 +143,8 @@ opts_start_struct(Visitor *v, void **obj, const char *kind, if (ov->opts_root->id != NULL) { ov->fake_id_opt = g_malloc0(sizeof *ov->fake_id_opt); - ov->fake_id_opt->name = "id"; - ov->fake_id_opt->str = ov->opts_root->id; + ov->fake_id_opt->name = g_strdup("id"); + ov->fake_id_opt->str = g_strdup(ov->opts_root->id); opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt); } } @@ -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); + } ov->fake_id_opt = NULL; } diff --git a/util/qemu-option.c b/util/qemu-option.c index d5e80da..eab5102 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); - g_free((/* !const */ char*)opt->name); - g_free((/* !const */ char*)opt->str); + g_free(opt->name); + g_free(opt->str); g_free(opt); }
Signed-off-by: Chunyan Liu <cyliu@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(-)