Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor

[Luiz Capitulino <lcapitulino@redhat.com>]

On Sat, 14 Jul 2012 00:48:58 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/13/12 18:51, Luiz Capitulino wrote:
> > On Wed, 13 Jun 2012 10:22:36 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
> 
> >> Repeating an optarg is supported;
> >
> > I see that the current code supports this too, but why? Something
> > like this should fail:
> >
> >  -netdev type=tap,vhost=on,vhost=off,id=guest1,script=qemu-ifup-switch
> 
> > Also, you're using a queue to support the repeating of optargs,
> > right? I think this could be simplified if we just don't support
> > that.
> 
> I hate repeated options with a passion, but SLIRP's hostfwd and guestfwd
> depend on repetition.

Oh, I see. I think it would be nicer to have a flag for this in QemuOptsList,
but that's unrelated to this series.

> > This doesn't insert the opts id into the hash, so opts_type_str()
> > will fail to find the id when the generated code visits it here:
> >
> > void visit_type_Netdev(Visitor *m, Netdev ** obj, const char *name, Error **errp)
> > {
> >     if (!error_is_set(errp)) {
> >         Error *err = NULL;
> >         visit_start_struct(m, (void **)obj, "Netdev", name, sizeof(Netdev), &err);
> >         if (!err) {
> >             assert(!obj || *obj);
> >             visit_type_str(m, obj ? &(*obj)->id : NULL, "id", &err); <---------
> > [...]
> >
> 
> *groan*
> 
> You're right. opts_do_parse() makes an exception with "id" and doesn't
> call opt_set() for any occurrence of it. Would you accept the attached
> fix, split up and squashed into previous parts appropriately?

I've glanced at it and it looked fine, but I'll wait for v3 to do a full
review again.