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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Mon, 21 Jun 2010 10:12:06 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> 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

"can't have" for crying out loud!

>> 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".

Workable, I think.

> 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.

Just like any other type, the O-type tells us how to parse a single
positional argument.

> 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.

Should be good enough for the initial commit.