Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 02 Jun 2010 09:22:40 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> There's more...

 Good!

> Luiz Capitulino <lcapitulino@redhat.com> writes:

[...]

> > +static void check_mandatory_args(const char *cmd_arg_name,
> > +                                 QObject *obj, void *opaque)
> > +{
> > +    QString *type;
> > +    QMPArgCheckRes *res = opaque;
> > +
> > +    if (res->result < 0) {
> > +        /* report only the first error */
> > +        return;
> > +    }
> > +
> > +    type = qobject_to_qstring(obj);
> > +    assert(type != NULL);
> > +
> > +    if (qstring_get_str(type)[0] == 'O') {
> > +        QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
> > +        assert(opts_list);
> > +        res->result = check_opts(opts_list, res->qdict);
> > +        res->skip = 1;
> > +    } else if (qstring_get_str(type)[0] != '-' &&
> > +               qstring_get_str(type)[1] != '?' &&
> > +               !qdict_haskey(res->qdict, cmd_arg_name)) {
> > +        res->result = -1;
> 
> This is a sign that the iterator needs a way to return a value.
> 
> Check out qemu_opts_foreach(), it can break and return a value.

 Ah, that's good, I was wondering how I could do that but couldn't
find a good way.

[...]

> Higher order functions rock.  But C is too static and limited for
> elegant use of higher order functions.  Means to construct loops are
> usually more convenient to use, and yield more readable code.
> 
> I find the use of qdict_iter() here quite tortuous: you define a
> separate iterator function, which you can't put next to its use.  You
> need to jump back and forth between the two places to understand what
> the loop does.  You define a special data structure just to pass
> arguments and results through qdict_iter().
> 
> Let me try to sketch the alternative:
> 
> static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
> {
>     QDict *cmd_args;
>     int res = 0, skip = 0;
>     QDictEntry *ent;
> 
>     cmd_args = qdict_from_args_type(cmd->args_type);
> 
>     for (ent = qdict_first_entry(cmd_args); ent; ent = qdict_next_entry(ent) {

 I thought about doing something similar a while ago, but I dislike it for
two reasons:

  1. I don't think the notion of 'first' and 'next' apply for dicts. One may
     argue that the iterator has the same issue, but it's implicit

  2. QDictEntry shouldn't be part of the public interface, we should be
     using forward declaration there (although I'm not sure whether this is
     possible with a typedef)

 I think having qdict_foreach() will improve things already.