2014-03-11 5:44 GMT+08:00 Eric Blake <ebl...@redhat.com>:
> 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?
>
Well, now I think it caused by drv (vvfat.c) and proto_drv (raw-posix.c) not
consistent, one is using QemuOpts, the other is using QEMUOptionParameter.
That will cause create_opts became NULL, then when passing a size option,
qemu_opt_set_size is OK, but later qemu_opt_get_size will segment fault.
After solving the drv/proto_drv consistent issue, this problem won't happen.
So, in this patch series, this place could not be changed.
( But with opts_accept_any, I still think this place may bring problem some
time
in future.)
>
> 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
>
>
