| Message ID | 1375428840-5275-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/02/2013 01:34 AM, Markus Armbruster wrote: > Commit 6d4cd40 fixed qemu_opts_set_defaults() for an existing corner > case, but broke it for another one that can't be reached in current > code. > > > Not reachable, because we never pass lists with !list->merge_lists to > qemu_opts_set_defaults(). > > Guard against possible (if unlikely) future misuse with assert(). > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > util/qemu-option.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > + * This code doesn't work for defaults && !list->merge_lists: when > + * params has no id=, and list has an element with !opts->id, it > + * appends a new element instead of returning the existing opts. > + * However, we got no use for this case. Guard against possible s/got/have/ Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 08/02/2013 01:34 AM, Markus Armbruster wrote: >> Commit 6d4cd40 fixed qemu_opts_set_defaults() for an existing corner >> case, but broke it for another one that can't be reached in current >> code. >> > >> >> Not reachable, because we never pass lists with !list->merge_lists to >> qemu_opts_set_defaults(). >> >> Guard against possible (if unlikely) future misuse with assert(). >> >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> util/qemu-option.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) > >> + * This code doesn't work for defaults && !list->merge_lists: when >> + * params has no id=, and list has an element with !opts->id, it >> + * appends a new element instead of returning the existing opts. >> + * However, we got no use for this case. Guard against possible > > s/got/have/ I'd be fine with fixing this on commit. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
Applied. Thanks. Regards, Anthony Liguori
diff --git a/util/qemu-option.c b/util/qemu-option.c index 7a1552a..4ebdc4c 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -928,6 +928,15 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params, get_opt_value(value, sizeof(value), p+4); id = value; } + + /* + * This code doesn't work for defaults && !list->merge_lists: when + * params has no id=, and list has an element with !opts->id, it + * appends a new element instead of returning the existing opts. + * However, we got no use for this case. Guard against possible + * (if unlikely) future misuse: + */ + assert(!defaults || list->merge_lists); opts = qemu_opts_create(list, id, !defaults, &local_err); if (opts == NULL) { if (error_is_set(&local_err)) {
Commit 6d4cd40 fixed qemu_opts_set_defaults() for an existing corner case, but broke it for another one that can't be reached in current code. Quote from its commit message: I believe [opts_parse()] attempts to do the following: If options don't yet exist, create new options Else, if defaults, modify the existing options Else, if list->merge_lists, modify the existing options Else, fail The only caller that passes true for defaults is qemu_opts_set_defaults(). The commit message then claims: A straightforward call of qemu_opts_create() does exactly that. Wrong. When !list->merge_lists, and the option string doesn't contain id=, and options without ID exist, then we don't actually modify the existing options, we create new ones. Not reachable, because we never pass lists with !list->merge_lists to qemu_opts_set_defaults(). Guard against possible (if unlikely) future misuse with assert(). Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- util/qemu-option.c | 9 +++++++++ 1 file changed, 9 insertions(+)