There's more... Luiz Capitulino <lcapitul...@redhat.com> writes:
> This commit introduces the first half of qmp_check_client_args(), > which is the new client argument checker. > > It's introduced on top of the existing code, so that there are > no regressions during the transition. > > It works this way: the command's args_type field (from > qemu-monitor.hx) is transformed into a qdict. Then we iterate > over it checking whether each mandatory argument has been > provided by the client. > > All comunication between the iterator and its caller is done > via the new QMPArgCheckRes type. > > Next commit adds the second half of this work: type checking > and invalid argument detection. > > Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com> > --- > monitor.c | 107 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 107 insertions(+), 0 deletions(-) > > diff --git a/monitor.c b/monitor.c > index bc3cc18..47a0da8 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4259,6 +4259,108 @@ static int invalid_qmp_mode(const Monitor *mon, const > char *cmd_name) > return (qmp_cmd_mode(mon) ? is_cap : !is_cap); > } > > +typedef struct QMPArgCheckRes { > + int result; > + int skip; > + QDict *qdict; > +} QMPArgCheckRes; > + > +/* > + * Check if client passed all mandatory args > + */ > +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. > + qerror_report(QERR_MISSING_PARAMETER, cmd_arg_name); > + } > +} > + > +static QDict *qdict_from_args_type(const char *args_type) > +{ > + int i; > + QDict *qdict; > + QString *key, *type, *cur_qs; > + > + assert(args_type != NULL); > + > + qdict = qdict_new(); > + > + if (args_type == NULL || args_type[0] == '\0') { > + /* no args, empty qdict */ > + goto out; > + } > + > + key = qstring_new(); > + type = qstring_new(); > + > + cur_qs = key; > + > + for (i = 0;; i++) { > + switch (args_type[i]) { > + case ',': > + case '\0': > + qdict_put(qdict, qstring_get_str(key), type); > + QDECREF(key); > + if (args_type[i] == '\0') { > + goto out; > + } > + type = qstring_new(); /* qdict has ref */ > + cur_qs = key = qstring_new(); > + break; > + case ':': > + cur_qs = type; > + break; > + default: > + qstring_append_chr(cur_qs, args_type[i]); > + break; > + } > + } > + > +out: > + return qdict; > +} > + > +/* > + * Client argument checking rules: > + * > + * 1. Client must provide all mandatory arguments > + */ > +static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args) > +{ > + QDict *cmd_args; > + QMPArgCheckRes res = { .result = 0, .skip = 0 }; > + > + cmd_args = qdict_from_args_type(cmd->args_type); > + > + res.qdict = client_args; > + qdict_iter(cmd_args, check_mandatory_args, &res); > + > + /* TODO: Check client args type */ > + > + QDECREF(cmd_args); > + return res.result; > +} 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) { type = qobject_to_qstring(ent->value); assert(type != NULL); if (qstring_get_str(type)[0] == 'O') { QemuOptsList *opts_list = qemu_find_opts(ent->key); assert(opts_list); res = check_opts(opts_list, cmd_args); skip = 1; } else if (qstring_get_str(type)[0] != '-' && qstring_get_str(type)[1] != '?' && !qdict_haskey(cmd_args, ent->key)) { qerror_report(QERR_MISSING_PARAMETER, ent->key); res = -1; break; } } /* TODO: Check client args type */ QDECREF(cmd_args); return res; } > + > static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) > { > int err; > @@ -4334,6 +4436,11 @@ static void handle_qmp_command(JSONMessageParser > *parser, QList *tokens) > > QDECREF(input); > > + err = qmp_check_client_args(cmd, args); > + if (err < 0) { > + goto err_out; > + } > + > err = monitor_check_qmp_args(cmd, args); > if (err < 0) { > goto err_out;