Luiz Capitulino <lcapitul...@redhat.com> writes:
> On Mon, 21 Jun 2010 10:12:06 +0200
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitul...@redhat.com> writes:
>>
>> > On Wed, 02 Jun 2010 09:31:24 +0200
>> > Markus Armbruster <arm...@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitul...@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
"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.
