[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checkin
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code |
Date: |
Wed, 02 Jun 2010 09:22:40 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
There's more...
Luiz Capitulino <address@hidden> 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 <address@hidden>
> ---
> 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;
[Qemu-devel] [PATCH 2/9] Monitor: handle optional '-' arg as a bool, Luiz Capitulino, 2010/06/01
[Qemu-devel] [PATCH 5/9] QMP: Drop old client argument checker, Luiz Capitulino, 2010/06/01
[Qemu-devel] [PATCH 6/9] QMP: check_opts(): Minor cleanup, Luiz Capitulino, 2010/06/01
[Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code, Luiz Capitulino, 2010/06/01