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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Wed, 02 Jun 2010 09:31:24 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> 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.

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.

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.

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.

Complication: you can have a QemuOptsList accept arbitrary arguments,
regardless of their name.  Checking them is left to its consumer.

The way it's currently done (empty desc[] member), such an O-type can't
declare *any* names when it accepts arbitrary arguments.

Obviously, we can't have more than one O-type parameter accept arbitrary
arguments, because we wouldn't know which O-type should get them.

An O-type accepting arbitrary arguments should receive the "left-overs",
i.e. any arguments that can't be matched to named parameters.

Summary:

* Parameter names must be unique, whether they are defined directly in
  args_type or included by an O-type.

* We can't have multiple O-types accepting arbitrary arguments.

* If we have an O-type accepting arbitrary arguments, it receives any
  arguments that can't be matched to named parameters.

* I'd be fine with the restriction to a single O-type.

>  * Does this work with the current argument checker?

I added O-types in the device_add series.  I implemented only what
device_add needs, because that series was already awkwardly long.

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.