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