| Message ID | 1394436721-21812-4-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
On 03/10/2014 01:31 AM, Chunyan Liu wrote: > Improve opt_get and opt_set group of functions. For opt_get, check and handle > NULL input; for opt_set, when set to an existing option, rewrite the option > with new value. > > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > include/qemu/option_int.h | 4 +-- > util/qemu-option.c | 81 +++++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 69 insertions(+), 16 deletions(-) > > > +++ 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; You are losing the 'const' here... > @@ -704,6 +730,13 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value, > return; > } > > + opt = qemu_opt_find(opts, name); > + if (opt) { > + g_free((char *)opt->str); ...which means the cast is pointless here. Hmm. This means that you are giving opt_set() the behavior of 'last version wins', by silently overwriting earlier versions. If I'm understanding the existing code correctly, the previous behavior was that calling opt_set twice in a row on the same name would inject BOTH names into 'opts', but then subsequent lookups on opts would find the FIRST hit. Doesn't that mean this is a semantic change: qemu -opt key=value1,key=value2 would previously set key to value1, but now sets key to value2. Is it intentional? If so, document it in the commit message. Or am I missing something, where we already reject duplicates, in which case, your new code is currently unreachable, and your commit message could do with more explanation of why you are adding something that later patches will make use of. Also, I'd feel a LOT better if we FIRST had a testsuite that covered what QemuOpts is _supposed_ to do before we then patch it,so that we aren't getting surprised by silent changes in behavior. It's okay to write a test that shows old behavior, then tweak the test in the patch that updates the behavior along with the justification of why the new behavior is nicer. > @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value, > int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > + opt = qemu_opt_find(opts, name); > + if (opt) { > + g_free((char *)opt->str); Another pointless cast. > > + opt = qemu_opt_find(opts, name); > + if (opt) { > + g_free((char *)opt->str); and another.
On 03/10/2014 02:29 PM, Eric Blake wrote: >> + opt = qemu_opt_find(opts, name); >> + if (opt) { >> + g_free((char *)opt->str); > > ...which means the cast is pointless here. > > Hmm. This means that you are giving opt_set() the behavior of 'last > version wins', by silently overwriting earlier versions. If I'm > understanding the existing code correctly, the previous behavior was > that calling opt_set twice in a row on the same name would inject BOTH > names into 'opts', but then subsequent lookups on opts would find the > FIRST hit. Doesn't that mean this is a semantic change: > > qemu -opt key=value1,key=value2 > > would previously set key to value1, but now sets key to value2. I've played with this a bit more, and now am more confused. QemuOpts is a LOT to comprehend. Pre-patch, 'qemu-system-x86_64 -nodefaults -machine type=none,type-noone' displayed a help message about unknown machine type "noone", while swapping type=noone,type=none proceeded with the 'none' type. So the last version silently won, which was not the behavior I had predicted. Post-patch, I get a compilation error (so how did you test your patch?): qapi/opts-visitor.c: In function ‘opts_start_struct’: qapi/opts-visitor.c:146:31: error: assignment discards ‘const’ qualifier from pointer target type [-Werror] ov->fake_id_opt->name = "id"; ^ If I press on in spite of that warning, then I get the same behavior where the last type= still wins on behavior. So I'm not sure how it all worked, but at least behavior wise, my one test didn't uncover a regression. Still, I'd feel a LOT better with a testsuite of what QemuOpts is supposed to be able to do. tests/test-opts-visitor.c was the only file in tests/ that even mentions QemuOpts. >> @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value, >> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > >> + opt = qemu_opt_find(opts, name); >> + if (opt) { >> + g_free((char *)opt->str); > > Another pointless cast. Maybe not pointless, if you end up not removing the const in the struct declaration due to the compile error; but that brings me back to my earlier question - since the compiler error proves that we have places that are assigning compile-time string constants into the name field, we must NOT call g_free on those copies - how does your code distinguish between a QemuOpt that is built up by mallocs, vs. one that is described by compile-time constants?
2014-03-11 5:21 GMT+08:00 Eric Blake <eblake@redhat.com>: > On 03/10/2014 02:29 PM, Eric Blake wrote: > > >> + opt = qemu_opt_find(opts, name); > >> + if (opt) { > >> + g_free((char *)opt->str); > > > > ...which means the cast is pointless here. > > > > Hmm. This means that you are giving opt_set() the behavior of 'last > > version wins', by silently overwriting earlier versions. If I'm > > understanding the existing code correctly, the previous behavior was > > that calling opt_set twice in a row on the same name would inject BOTH > > names into 'opts', but then subsequent lookups on opts would find the > > FIRST hit. Doesn't that mean this is a semantic change: > > > > qemu -opt key=value1,key=value2 > > > > would previously set key to value1, but now sets key to value2. > > I've played with this a bit more, and now am more confused. QemuOpts is > a LOT to comprehend. > > Pre-patch, 'qemu-system-x86_64 -nodefaults -machine > type=none,type-noone' displayed a help message about unknown machine > type "noone", while swapping type=noone,type=none proceeded with the > 'none' type. So the last version silently won, which was not the > behavior I had predicted. > > Post-patch, I get a compilation error (so how did you test your patch?): > Mostly tested ./qemu-img commands where QEMUOptionParameter is used. I really didn't think of test QemuOpts fully, and about the test suite, I have no full knowledge about how many things need to be tested, how many things need to be covered? > qapi/opts-visitor.c: In function ‘opts_start_struct’: > qapi/opts-visitor.c:146:31: error: assignment discards ‘const’ qualifier > from pointer target type [-Werror] > ov->fake_id_opt->name = "id"; > ^ > This is a place needed to be changed but missing, since name and str changed to be (char *), here should be g_strdup("id"). Due to my configuration, it appears as a warning instead of error which is missed. Updated 3/25. > If I press on in spite of that warning, then I get the same behavior > where the last type= still wins on behavior. So I'm not sure how it all > worked, but at least behavior wise, my one test didn't uncover a > regression. > > Still, I'd feel a LOT better with a testsuite of what QemuOpts is > supposed to be able to do. tests/test-opts-visitor.c was the only file > in tests/ that even mentions QemuOpts. > > > >> @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char > *name, const char *value, > >> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > > > >> + opt = qemu_opt_find(opts, name); > >> + if (opt) { > >> + g_free((char *)opt->str); > > > > Another pointless cast. > > Maybe not pointless, if you end up not removing the const in the struct > declaration due to the compile error; but that brings me back to my > earlier question - since the compiler error proves that we have places > that are assigning compile-time string constants into the name field, we > must NOT call g_free on those copies - how does your code distinguish > between a QemuOpt that is built up by mallocs, vs. one that is described > by compile-time constants? > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
Hi, On Tue, Mar 11, 2014 at 03:26:51PM +0800, Chunyan Liu wrote: > 2014-03-11 5:21 GMT+08:00 Eric Blake <eblake@redhat.com>: > > > On 03/10/2014 02:29 PM, Eric Blake wrote: > > > > >> + opt = qemu_opt_find(opts, name); > > >> + if (opt) { > > >> + g_free((char *)opt->str); > > > > > > ...which means the cast is pointless here. > > > > > > Hmm. This means that you are giving opt_set() the behavior of 'last > > > version wins', by silently overwriting earlier versions. If I'm > > > understanding the existing code correctly, the previous behavior was > > > that calling opt_set twice in a row on the same name would inject BOTH > > > names into 'opts', but then subsequent lookups on opts would find the > > > FIRST hit. Doesn't that mean this is a semantic change: > > > > > > qemu -opt key=value1,key=value2 > > > > > > would previously set key to value1, but now sets key to value2. > > > > I've played with this a bit more, and now am more confused. QemuOpts is > > a LOT to comprehend. > > > > Pre-patch, 'qemu-system-x86_64 -nodefaults -machine > > type=none,type-noone' displayed a help message about unknown machine > > type "noone", while swapping type=noone,type=none proceeded with the > > 'none' type. So the last version silently won, which was not the > > behavior I had predicted. > > > > Post-patch, I get a compilation error (so how did you test your patch?): > > > > Mostly tested ./qemu-img commands where QEMUOptionParameter is used. > I really didn't think of test QemuOpts fully, and about the test suite, I > have no full > knowledge about how many things need to be tested, how many things need to > be > covered? The testsuite should test the QemuOpts implementation, not the current users. Regards...
2014-03-11 5:21 GMT+08:00 Eric Blake <eblake@redhat.com>: > On 03/10/2014 02:29 PM, Eric Blake wrote: > > >> + opt = qemu_opt_find(opts, name); > >> + if (opt) { > >> + g_free((char *)opt->str); > > > > ...which means the cast is pointless here. > > > > Hmm. This means that you are giving opt_set() the behavior of 'last > > version wins', by silently overwriting earlier versions. If I'm > > understanding the existing code correctly, the previous behavior was > > that calling opt_set twice in a row on the same name would inject BOTH > > names into 'opts', but then subsequent lookups on opts would find the > > FIRST hit. Doesn't that mean this is a semantic change: > > > > qemu -opt key=value1,key=value2 > > > > would previously set key to value1, but now sets key to value2. > > I've played with this a bit more, and now am more confused. QemuOpts is > a LOT to comprehend. > > Pre-patch, 'qemu-system-x86_64 -nodefaults -machine > type=none,type-noone' displayed a help message about unknown machine > type "noone", while swapping type=noone,type=none proceeded with the > 'none' type. So the last version silently won, which was not the > behavior I had predicted. > In qemu_opt_find(), it uses: QTAILQ_FOREACH_REVERSE(), so that means find the last setting, the same result with replacement. > Post-patch, I get a compilation error (so how did you test your patch?): > > qapi/opts-visitor.c: In function ‘opts_start_struct’: > qapi/opts-visitor.c:146:31: error: assignment discards ‘const’ qualifier > from pointer target type [-Werror] > ov->fake_id_opt->name = "id"; > ^ > > If I press on in spite of that warning, then I get the same behavior > where the last type= still wins on behavior. So I'm not sure how it all > worked, but at least behavior wise, my one test didn't uncover a > regression. > > Still, I'd feel a LOT better with a testsuite of what QemuOpts is > supposed to be able to do. tests/test-opts-visitor.c was the only file > in tests/ that even mentions QemuOpts. > > > >> @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char > *name, const char *value, > >> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > > > >> + opt = qemu_opt_find(opts, name); > >> + if (opt) { > >> + g_free((char *)opt->str); > > > > Another pointless cast. > > Maybe not pointless, if you end up not removing the const in the struct > declaration due to the compile error; but that brings me back to my > earlier question - since the compiler error proves that we have places > that are assigning compile-time string constants into the name field, we > must NOT call g_free on those copies - how does your code distinguish > between a QemuOpt that is built up by mallocs, vs. one that is described > by compile-time constants? > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
Hi Chunyan, On Tue, Mar 11, 2014 at 09:00:04PM +0000, Leandro Dorileo wrote: > Hi, > > On Tue, Mar 11, 2014 at 03:26:51PM +0800, Chunyan Liu wrote: > > 2014-03-11 5:21 GMT+08:00 Eric Blake <eblake@redhat.com>: > > > > > On 03/10/2014 02:29 PM, Eric Blake wrote: > > > > > > >> + opt = qemu_opt_find(opts, name); > > > >> + if (opt) { > > > >> + g_free((char *)opt->str); > > > > > > > > ...which means the cast is pointless here. > > > > > > > > Hmm. This means that you are giving opt_set() the behavior of 'last > > > > version wins', by silently overwriting earlier versions. If I'm > > > > understanding the existing code correctly, the previous behavior was > > > > that calling opt_set twice in a row on the same name would inject BOTH > > > > names into 'opts', but then subsequent lookups on opts would find the > > > > FIRST hit. Doesn't that mean this is a semantic change: > > > > > > > > qemu -opt key=value1,key=value2 > > > > > > > > would previously set key to value1, but now sets key to value2. > > > > > > I've played with this a bit more, and now am more confused. QemuOpts is > > > a LOT to comprehend. > > > > > > Pre-patch, 'qemu-system-x86_64 -nodefaults -machine > > > type=none,type-noone' displayed a help message about unknown machine > > > type "noone", while swapping type=noone,type=none proceeded with the > > > 'none' type. So the last version silently won, which was not the > > > behavior I had predicted. > > > > > > Post-patch, I get a compilation error (so how did you test your patch?): > > > > > > > Mostly tested ./qemu-img commands where QEMUOptionParameter is used. > > I really didn't think of test QemuOpts fully, and about the test suite, I > > have no full > > knowledge about how many things need to be tested, how many things need to > > be > > covered? > > The testsuite should test the QemuOpts implementation, not the current users. I have just posted a patch introducing a basic QemuOpt testsuite, we can use it as a start. Regards...
Hi, On Mon, Mar 10, 2014 at 03:31:39PM +0800, Chunyan Liu wrote: > Improve opt_get and opt_set group of functions. For opt_get, check and handle > NULL input; for opt_set, when set to an existing option, rewrite the option > with new value. > > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > include/qemu/option_int.h | 4 +-- > util/qemu-option.c | 81 +++++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 69 insertions(+), 16 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; > > const QemuOptDesc *desc; > union { > diff --git a/util/qemu-option.c b/util/qemu-option.c > index 65d1c22..c7639e8 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -558,8 +558,13 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name) > > const char *qemu_opt_get(QemuOpts *opts, const char *name) > { > - QemuOpt *opt = qemu_opt_find(opts, name); > + QemuOpt *opt; > > + if (opts == NULL) { > + return NULL; > + } I understand you want to prevent a segfault in qemu_opt_find(), and I'm fine with that, but these checks make me wonder why you decided to change it, aren't you hiding some broken caller? Btw, you could change qemu_opt_find() and check if opts != NULL there and not introduce all these opts == NULL qemu_opt_find() pairs. -- Dorileo > + > + opt = qemu_opt_find(opts, name); > if (!opt) { > const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); > if (desc && desc->def_value_str) { > @@ -583,7 +588,13 @@ bool qemu_opt_has_help_opt(QemuOpts *opts) > > bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval) > { > - QemuOpt *opt = qemu_opt_find(opts, name); > + QemuOpt *opt; > + > + if (opts == NULL) { > + return defval; > + } > + > + opt = qemu_opt_find(opts, name); > > if (opt == NULL) { > const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); > @@ -598,7 +609,13 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval) > > uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval) > { > - QemuOpt *opt = qemu_opt_find(opts, name); > + QemuOpt *opt; > + > + if (opts == NULL) { > + return defval; > + } > + > + opt = qemu_opt_find(opts, name); > > if (opt == NULL) { > const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); > @@ -614,8 +631,13 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval) > > uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval) > { > - QemuOpt *opt = qemu_opt_find(opts, name); > + QemuOpt *opt; > > + if (opts == NULL) { > + return defval; > + } > + > + opt = qemu_opt_find(opts, name); > if (opt == NULL) { > const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); > if (desc && desc->def_value_str) { > @@ -652,6 +674,10 @@ static void qemu_opt_parse(QemuOpt *opt, Error **errp) > > static void qemu_opt_del(QemuOpt *opt) > { > + if (opt == NULL) { > + return; > + } > + > QTAILQ_REMOVE(&opt->opts->head, opt, next); > g_free((/* !const */ char*)opt->name); > g_free((/* !const */ char*)opt->str); > @@ -704,6 +730,13 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value, > return; > } > > + opt = qemu_opt_find(opts, name); > + if (opt) { > + g_free((char *)opt->str); > + opt->str = g_strdup(value); > + return; > + } > + > opt = g_malloc0(sizeof(*opt)); > opt->name = g_strdup(name); > opt->opts = opts; > @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value, > int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > { > QemuOpt *opt; > - const QemuOptDesc *desc = opts->list->desc; > + const QemuOptDesc *desc; > > - opt = g_malloc0(sizeof(*opt)); > - opt->desc = find_desc_by_name(desc, name); > - if (!opt->desc && !opts_accepts_any(opts)) { > + desc = find_desc_by_name(opts->list->desc, name); > + if (!desc && !opts_accepts_any(opts)) { > qerror_report(QERR_INVALID_PARAMETER, name); > - g_free(opt); > return -1; > } > > + opt = qemu_opt_find(opts, name); > + if (opt) { > + g_free((char *)opt->str); > + opt->value.boolean = val; > + opt->str = g_strdup(val ? "on" : "off"); > + return 0; > + } > + > + opt = g_malloc0(sizeof(*opt)); > + opt->desc = desc; > opt->name = g_strdup(name); > opt->opts = opts; > opt->value.boolean = !!val; > @@ -766,16 +807,24 @@ int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val) > { > QemuOpt *opt; > - const QemuOptDesc *desc = opts->list->desc; > + const QemuOptDesc *desc; > > - opt = g_malloc0(sizeof(*opt)); > - opt->desc = find_desc_by_name(desc, name); > - if (!opt->desc && !opts_accepts_any(opts)) { > + desc = find_desc_by_name(opts->list->desc, name); > + if (!desc && !opts_accepts_any(opts)) { > qerror_report(QERR_INVALID_PARAMETER, name); > - g_free(opt); > return -1; > } > > + opt = qemu_opt_find(opts, name); > + if (opt) { > + g_free((char *)opt->str); > + opt->value.uint = val; > + opt->str = g_strdup_printf("%" PRId64, val); > + return 0; > + } > + > + opt = g_malloc0(sizeof(*opt)); > + opt->desc = desc; > opt->name = g_strdup(name); > opt->opts = opts; > opt->value.uint = val; > @@ -910,6 +959,10 @@ void qemu_opts_del(QemuOpts *opts) > { > QemuOpt *opt; > > + if (opts == NULL) { > + return; > + } > + > for (;;) { > opt = QTAILQ_FIRST(&opts->head); > if (opt == NULL) > -- > 1.7.12.4 > >
2014-03-17 5:19 GMT+08:00 Leandro Dorileo <l@dorileo.org>: > Hi Chunyan, > > On Tue, Mar 11, 2014 at 09:00:04PM +0000, Leandro Dorileo wrote: > > Hi, > > > > On Tue, Mar 11, 2014 at 03:26:51PM +0800, Chunyan Liu wrote: > > > 2014-03-11 5:21 GMT+08:00 Eric Blake <eblake@redhat.com>: > > > > > > > On 03/10/2014 02:29 PM, Eric Blake wrote: > > > > > > > > >> + opt = qemu_opt_find(opts, name); > > > > >> + if (opt) { > > > > >> + g_free((char *)opt->str); > > > > > > > > > > ...which means the cast is pointless here. > > > > > > > > > > Hmm. This means that you are giving opt_set() the behavior of > 'last > > > > > version wins', by silently overwriting earlier versions. If I'm > > > > > understanding the existing code correctly, the previous behavior > was > > > > > that calling opt_set twice in a row on the same name would inject > BOTH > > > > > names into 'opts', but then subsequent lookups on opts would find > the > > > > > FIRST hit. Doesn't that mean this is a semantic change: > > > > > > > > > > qemu -opt key=value1,key=value2 > > > > > > > > > > would previously set key to value1, but now sets key to value2. > > > > > > > > I've played with this a bit more, and now am more confused. > QemuOpts is > > > > a LOT to comprehend. > > > > > > > > Pre-patch, 'qemu-system-x86_64 -nodefaults -machine > > > > type=none,type-noone' displayed a help message about unknown machine > > > > type "noone", while swapping type=noone,type=none proceeded with the > > > > 'none' type. So the last version silently won, which was not the > > > > behavior I had predicted. > > > > > > > > Post-patch, I get a compilation error (so how did you test your > patch?): > > > > > > > > > > Mostly tested ./qemu-img commands where QEMUOptionParameter is used. > > > I really didn't think of test QemuOpts fully, and about the test > suite, I > > > have no full > > > knowledge about how many things need to be tested, how many things > need to > > > be > > > covered? > > > > The testsuite should test the QemuOpts implementation, not the current > users. > > I have just posted a patch introducing a basic QemuOpt testsuite, we can > use it > as a start. > Thanks a lot, I've seen that. > > Regards... > > -- > Leandro Dorileo > >
2014-03-18 3:58 GMT+08:00 Leandro Dorileo <l@dorileo.org>: > Hi, > > On Mon, Mar 10, 2014 at 03:31:39PM +0800, Chunyan Liu wrote: > > Improve opt_get and opt_set group of functions. For opt_get, check and > handle > > NULL input; for opt_set, when set to an existing option, rewrite the > option > > with new value. > > > > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > > --- > > include/qemu/option_int.h | 4 +-- > > util/qemu-option.c | 81 > +++++++++++++++++++++++++++++++++++++++-------- > > 2 files changed, 69 insertions(+), 16 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; > > > > const QemuOptDesc *desc; > > union { > > diff --git a/util/qemu-option.c b/util/qemu-option.c > > index 65d1c22..c7639e8 100644 > > --- a/util/qemu-option.c > > +++ b/util/qemu-option.c > > @@ -558,8 +558,13 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const > char *name) > > > > const char *qemu_opt_get(QemuOpts *opts, const char *name) > > { > > - QemuOpt *opt = qemu_opt_find(opts, name); > > + QemuOpt *opt; > > > > + if (opts == NULL) { > > + return NULL; > > + } > > > > I understand you want to prevent a segfault in qemu_opt_find(), and I'm > fine with that, > but these checks make me wonder why you decided to change it, aren't you > hiding some > broken caller? > It should be broken in generating patches (so I thought we should add input check), although might not be broken at the end :) Maybe I could look at and try to exclude some changes that's not mandatory to just make this patch series work. Like: don't need to change assertion as in patch 4/25, don't need to replace existing setting in qemu_opt_set_* functions as in patch 3/25. > > Btw, you could change qemu_opt_find() and check if opts != NULL there and > not introduce > all these opts == NULL qemu_opt_find() pairs. > Could do. Either change qemu_opt_find or qemu_opt_get. I can update to change qemu_opt_find :) > > -- > Dorileo > > > + > > + opt = qemu_opt_find(opts, name); > > if (!opt) { > > const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, > name); > > if (desc && desc->def_value_str) { > > @@ -583,7 +588,13 @@ bool qemu_opt_has_help_opt(QemuOpts *opts) > > > > bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval) > > { > > - QemuOpt *opt = qemu_opt_find(opts, name); > > + QemuOpt *opt; > > + > > + if (opts == NULL) { > > + return defval; > > + } > > + > > + opt = qemu_opt_find(opts, name); > > > > if (opt == NULL) { > > const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, > name); > > @@ -598,7 +609,13 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char > *name, bool defval) > > > > uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t > defval) > > { > > - QemuOpt *opt = qemu_opt_find(opts, name); > > + QemuOpt *opt; > > + > > + if (opts == NULL) { > > + return defval; > > + } > > + > > + opt = qemu_opt_find(opts, name); > > > > if (opt == NULL) { > > const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, > name); > > @@ -614,8 +631,13 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const > char *name, uint64_t defval) > > > > uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t > defval) > > { > > - QemuOpt *opt = qemu_opt_find(opts, name); > > + QemuOpt *opt; > > > > + if (opts == NULL) { > > + return defval; > > + } > > + > > + opt = qemu_opt_find(opts, name); > > if (opt == NULL) { > > const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, > name); > > if (desc && desc->def_value_str) { > > @@ -652,6 +674,10 @@ static void qemu_opt_parse(QemuOpt *opt, Error > **errp) > > > > static void qemu_opt_del(QemuOpt *opt) > > { > > + if (opt == NULL) { > > + return; > > + } > > + > > QTAILQ_REMOVE(&opt->opts->head, opt, next); > > g_free((/* !const */ char*)opt->name); > > g_free((/* !const */ char*)opt->str); > > @@ -704,6 +730,13 @@ static void opt_set(QemuOpts *opts, const char > *name, const char *value, > > return; > > } > > > > + opt = qemu_opt_find(opts, name); > > + if (opt) { > > + g_free((char *)opt->str); > > + opt->str = g_strdup(value); > > + return; > > + } > > + > > opt = g_malloc0(sizeof(*opt)); > > opt->name = g_strdup(name); > > opt->opts = opts; > > @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char > *name, const char *value, > > int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > > { > > QemuOpt *opt; > > - const QemuOptDesc *desc = opts->list->desc; > > + const QemuOptDesc *desc; > > > > - opt = g_malloc0(sizeof(*opt)); > > - opt->desc = find_desc_by_name(desc, name); > > - if (!opt->desc && !opts_accepts_any(opts)) { > > + desc = find_desc_by_name(opts->list->desc, name); > > + if (!desc && !opts_accepts_any(opts)) { > > qerror_report(QERR_INVALID_PARAMETER, name); > > - g_free(opt); > > return -1; > > } > > > > + opt = qemu_opt_find(opts, name); > > + if (opt) { > > + g_free((char *)opt->str); > > + opt->value.boolean = val; > > + opt->str = g_strdup(val ? "on" : "off"); > > + return 0; > > + } > > + > > + opt = g_malloc0(sizeof(*opt)); > > + opt->desc = desc; > > opt->name = g_strdup(name); > > opt->opts = opts; > > opt->value.boolean = !!val; > > @@ -766,16 +807,24 @@ int qemu_opt_set_bool(QemuOpts *opts, const char > *name, bool val) > > int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val) > > { > > QemuOpt *opt; > > - const QemuOptDesc *desc = opts->list->desc; > > + const QemuOptDesc *desc; > > > > - opt = g_malloc0(sizeof(*opt)); > > - opt->desc = find_desc_by_name(desc, name); > > - if (!opt->desc && !opts_accepts_any(opts)) { > > + desc = find_desc_by_name(opts->list->desc, name); > > + if (!desc && !opts_accepts_any(opts)) { > > qerror_report(QERR_INVALID_PARAMETER, name); > > - g_free(opt); > > return -1; > > } > > > > + opt = qemu_opt_find(opts, name); > > + if (opt) { > > + g_free((char *)opt->str); > > + opt->value.uint = val; > > + opt->str = g_strdup_printf("%" PRId64, val); > > + return 0; > > + } > > + > > + opt = g_malloc0(sizeof(*opt)); > > + opt->desc = desc; > > opt->name = g_strdup(name); > > opt->opts = opts; > > opt->value.uint = val; > > @@ -910,6 +959,10 @@ void qemu_opts_del(QemuOpts *opts) > > { > > QemuOpt *opt; > > > > + if (opts == NULL) { > > + return; > > + } > > + > > for (;;) { > > opt = QTAILQ_FIRST(&opts->head); > > if (opt == NULL) > > -- > > 1.7.12.4 > > > > > >
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/util/qemu-option.c b/util/qemu-option.c index 65d1c22..c7639e8 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -558,8 +558,13 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name) const char *qemu_opt_get(QemuOpts *opts, const char *name) { - QemuOpt *opt = qemu_opt_find(opts, name); + QemuOpt *opt; + if (opts == NULL) { + return NULL; + } + + opt = qemu_opt_find(opts, name); if (!opt) { const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { @@ -583,7 +588,13 @@ bool qemu_opt_has_help_opt(QemuOpts *opts) bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval) { - QemuOpt *opt = qemu_opt_find(opts, name); + QemuOpt *opt; + + if (opts == NULL) { + return defval; + } + + opt = qemu_opt_find(opts, name); if (opt == NULL) { const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); @@ -598,7 +609,13 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval) uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval) { - QemuOpt *opt = qemu_opt_find(opts, name); + QemuOpt *opt; + + if (opts == NULL) { + return defval; + } + + opt = qemu_opt_find(opts, name); if (opt == NULL) { const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); @@ -614,8 +631,13 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval) uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval) { - QemuOpt *opt = qemu_opt_find(opts, name); + QemuOpt *opt; + if (opts == NULL) { + return defval; + } + + opt = qemu_opt_find(opts, name); if (opt == NULL) { const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { @@ -652,6 +674,10 @@ static void qemu_opt_parse(QemuOpt *opt, Error **errp) static void qemu_opt_del(QemuOpt *opt) { + if (opt == NULL) { + return; + } + QTAILQ_REMOVE(&opt->opts->head, opt, next); g_free((/* !const */ char*)opt->name); g_free((/* !const */ char*)opt->str); @@ -704,6 +730,13 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value, return; } + opt = qemu_opt_find(opts, name); + if (opt) { + g_free((char *)opt->str); + opt->str = g_strdup(value); + return; + } + opt = g_malloc0(sizeof(*opt)); opt->name = g_strdup(name); opt->opts = opts; @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value, int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) { QemuOpt *opt; - const QemuOptDesc *desc = opts->list->desc; + const QemuOptDesc *desc; - opt = g_malloc0(sizeof(*opt)); - opt->desc = find_desc_by_name(desc, name); - if (!opt->desc && !opts_accepts_any(opts)) { + desc = find_desc_by_name(opts->list->desc, name); + if (!desc && !opts_accepts_any(opts)) { qerror_report(QERR_INVALID_PARAMETER, name); - g_free(opt); return -1; } + opt = qemu_opt_find(opts, name); + if (opt) { + g_free((char *)opt->str); + opt->value.boolean = val; + opt->str = g_strdup(val ? "on" : "off"); + return 0; + } + + opt = g_malloc0(sizeof(*opt)); + opt->desc = desc; opt->name = g_strdup(name); opt->opts = opts; opt->value.boolean = !!val; @@ -766,16 +807,24 @@ int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val) { QemuOpt *opt; - const QemuOptDesc *desc = opts->list->desc; + const QemuOptDesc *desc; - opt = g_malloc0(sizeof(*opt)); - opt->desc = find_desc_by_name(desc, name); - if (!opt->desc && !opts_accepts_any(opts)) { + desc = find_desc_by_name(opts->list->desc, name); + if (!desc && !opts_accepts_any(opts)) { qerror_report(QERR_INVALID_PARAMETER, name); - g_free(opt); return -1; } + opt = qemu_opt_find(opts, name); + if (opt) { + g_free((char *)opt->str); + opt->value.uint = val; + opt->str = g_strdup_printf("%" PRId64, val); + return 0; + } + + opt = g_malloc0(sizeof(*opt)); + opt->desc = desc; opt->name = g_strdup(name); opt->opts = opts; opt->value.uint = val; @@ -910,6 +959,10 @@ void qemu_opts_del(QemuOpts *opts) { QemuOpt *opt; + if (opts == NULL) { + return; + } + for (;;) { opt = QTAILQ_FIRST(&opts->head); if (opt == NULL)