Re: [Qemu-devel] Handling the O-type

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 21 Jun 2010 10:12:06 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 02 Jun 2010 09:31:24 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >
> > [...]
> >
> >> >  static void check_mandatory_args(const char *cmd_arg_name,
> >> > @@ -4344,6 +4413,9 @@ out:
> >> >   * Client argument checking rules:
> >> >   *
> >> >   * 1. Client must provide all mandatory arguments
> >> > + * 2. Each argument provided by the client must be valid
> >> > + * 3. Each argument provided by the client must have the type expected
> >> > + *    by the command
> >> >   */
> >> >  static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
> >> >  {
> >> > @@ -4355,7 +4427,10 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
> >> >      res.qdict = client_args;
> >> >      qdict_iter(cmd_args, check_mandatory_args, &res);
> >> >  
> >> > -    /* TODO: Check client args type */
> >> > +    if (!res.result && !res.skip) {
> >> > +        res.qdict = cmd_args;
> >> > +        qdict_iter(client_args, check_client_args_type, &res);
> >> > +    }
> >> 
> >> What if we have both an O-type argument and other arguments?  Then the
> >> 'O' makes check_client_args_type() set res.skip, and we duly skip
> >> checking the other arguments here.
> >
> > I was working on this and it seems a bad idea to allow mixing O-type and
> > other monitor types*.
> >
> > The reason is that you can't easily tell if an argument passed by the client
> > is part of the O-type or the monitor type. We could workaround this by trying to
> > ensure that an argument exists only in one of them, but I really feel this will
> > get messy.
> >
> > I think we should disallow mixing O-type with other argument types and maintain
> > the skip trick, ie. skip any checking in the top level if the argument is an
> > O-type one.
> 
> If you're proposing "if you have an O-type parameter, then you can have
> any other parameters", then I disagree.  That's too big a hammer.

Not sure if this changes what you're trying to say here, but actually what
I'm saying is "if you have an O-type parameter, then argument checking is
up to you".

The best way to fix that is to do the other way around, ie. O-type should
also be checked by the new checker.

> The problem is to match actual arguments to formal parameters.
> 
> In HMP, the matching is positional.  No ambiguity as long as positions
> are clearly delimited.  A positional argument maybe an O-type, and
> within that argument, matching is by option name.

Ok, so the HMP parser can tell when an O-type sequence beings and ends,
right? By looking at the code, I have the impression it does.

In this case, the new checker should do the same. Should be possible, right?

> The big hammer restriction would make it impossible for a command to
> take both positional arguments and named arguments, unless you do the
> named arguments ad hoc instead of with an O-type.  Some commands already
> take both positional and named arguments: pci_add, drive_add,
> host_net_add.  Okay, those examples aren't exactly pinnacles of human
> interface design.  Still, it's an ugly restriction.
> 
> Multiple O-types in the same command are probably a bad idea, because
> the user would have to remember which option goes into what positional
> argument.
> 
> In QMP, the matching is by parameter name.  No ambiguity as long as the
> names are unique.  Therefore, all we need to disallow is non-unique
> parameter names.

Yes, if there's an easy way to do that I will do.

> Having an args_type define the same parameter name twice is a
> programming error.  It doesn't matter whether the name is right in the
> string, or buried in an O-type.

Sure, but it's error prone.

[...]

> Sooner or later we'll want to switch to a more structured encoding of
> parameters than the args_type string.  We might want to revise or ditch
> the use of QemuOptsList then.

Yes, and we have to decide what to do before we get there.

My suggestion is: if it's easy to do the O-type checking in the new checker,
then let's do it. Otherwise let's live with the limitation until we can
properly fix it.