On 03/10/2014 01:31 AM, Chunyan Liu wrote: > In qemu_opt_set functions, if desc doen't exist but opts_accepts_any is true, > it
s/doen't/doesn't/ I mentioned the same problem against v20. It is very depressing when review comments are not addressed. > won't report error, but can still alloc an opt for the option and save it. > However, after that, when doing qemu_opt_get, this option could be found in > opts > but opt->desc is NULL. This is correct, should not be treated as error. > > This patch would fix vvfat issue after changing to QemuOpts. > > Signed-off-by: Chunyan Liu <cy...@suse.com> > --- > util/qemu-option.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/util/qemu-option.c b/util/qemu-option.c > index c7639e8..df79235 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -603,7 +603,9 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, > bool defval) > } > return defval; > } > - assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL); > + if (opt->desc) { > + assert(opt->desc->type == QEMU_OPT_BOOL); > + } > return opt->value.boolean; I'm not sure I like this. opt->value is a union, but opt_set() does NOT populate the union when opts_accepts_any() fails. Previously, we were using opt->desc->type as the discriminator for which branch of the union is valid. But with your patch, if an option was set as a string, but then queried as a boolean, we may be reading bogus contents from the union. Or even worse, if someone sets the uint member of the union to 0x100000000 via qemu_opt_set_number(), then later calls qemu_opt_get_bool, the boolean member _might_ read as true on some platforms and false on others, depending on things such as host endianness. How is vvfat broken without this patch? That is, what specific option are you setting without specifying its type, that later triggers the assertion when you try to get the option via a specific type? I'm wondering if the fix should look more like: if (opt->desc) { assert(opt->desc->type == QEMU_OPT_BOOL); return opt->value.boolean; } else { code to parse opt->str } so that you are not dereferencing an undefined state of the union. > @@ -625,7 +627,9 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const char > *name, uint64_t defval) > } > return defval; > } > - assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER); > + if (opt->desc) { > + assert(opt->desc->type == QEMU_OPT_NUMBER); > + } > return opt->value.uint; > } > > @@ -645,7 +649,9 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char > *name, uint64_t defval) > } > return defval; > } > - assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE); > + if (opt->desc) { > + assert(opt->desc->type == QEMU_OPT_SIZE); > + } > return opt->value.uint; Same problem in these two spots. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature