| Message ID | 1275424897-32253-4-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Luiz Capitulino <lcapitulino@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 <lcapitulino@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; skip is write-only in this patch. > + 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; > + } This is a sign that the iterator needs a way to break the loop. > + > + 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); I doubt this is the right place for calling check_opts. Right now, it's only implemented for QemuOptsList with empty desc. A more complete version is in my tree (blockdev needs it). Looks like this: static int check_opts(QemuOptsList *opts_list, QDict *args) { QemuOptDesc *desc; CmdArgs cmd_args; for (desc = opts_list->desc; desc->name; desc++) { cmd_args_init(&cmd_args); cmd_args.optional = 1; switch (desc->type) { case QEMU_OPT_STRING: cmd_args.type = 's'; break; case QEMU_OPT_BOOL: cmd_args.type = '-'; break; case QEMU_OPT_NUMBER: case QEMU_OPT_SIZE: cmd_args.type = 'l'; break; } qstring_append(cmd_args.name, desc->name); if (check_arg(&cmd_args, args) < 0) { QDECREF(cmd_args.name); return -1; } QDECREF(cmd_args.name); } return 0; } > + 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; > + qerror_report(QERR_MISSING_PARAMETER, cmd_arg_name); > + } > +} [...]
There's more... Luiz Capitulino <lcapitulino@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 <lcapitulino@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;
On Wed, 02 Jun 2010 08:59:11 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: [...] > > + > > + 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); > > I doubt this is the right place for calling check_opts. Can you suggest the right place?
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.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 02 Jun 2010 09:22:40 +0200 > Markus Armbruster <armbru@redhat.com> wrote: [...] >> 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 Does the dirt under the carpet exist when nobody looks? Iterating over an unordered collection necessarily creates an order where none was before. It's the nature of iteration. Dressing it up as iterator + function argument doesn't change the basic fact[*]. > 2. QDictEntry shouldn't be part of the public interface, we should be > using forward declaration there No problem, just add qdict_ent_key() and qdict_ent_value(), and use them instead of operator ->. > (although I'm not sure whether this is > possible with a typedef) In qdict.h: typedef struct QDictEntry QDictEntry; In qdict.c: struct QDictEntry { ... }; > I think having qdict_foreach() will improve things already. I doubt it, but try and see :) [*] Unless the iterator gets fancy and calls the function argument concurrently. Hardly an option in primitive old C.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 02 Jun 2010 08:59:11 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Luiz Capitulino <lcapitulino@redhat.com> writes: > > [...] > >> > + >> > + 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); >> >> I doubt this is the right place for calling check_opts. > > Can you suggest the right place? It's related to check_arg(). I figure it should be dropped along with it in 5/9. The new checker needs to cope with 'O': 1. Client must provide all mandatory arguments 'O' options are optional, so there's nothing to do for check_mandatory_args(). 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 'O' comes in two flavours, like the underlying QemuOptsList: desc present (not yet implemented) and empty. Empty desc means we accept any option. check_client_args_type() needs to accept unknown arguments. Is this broken in your patch? Non-empty desc probably should be exploded by qdict_from_args_type(), i.e. instead of putting (NAME, "O") into the dictionary, put the contents of QemuOptsList's desc. Problem: key is obvious (desc->name), but value is not. Maybe you need to represent "expected type" differently, so you can cover both arg_type codes and the QemuOptType values. What its user really needs to know is the set of expected types. Why not put that, in a suitable encoding. I recommend to implement only empty desc here for now, and non-empty when we implement it. Would be nice if you could keep it in mind, so we don't have to dig up too much of the argument checker for it.
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; + 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; +} + 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;
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 <lcapitulino@redhat.com> --- monitor.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 107 insertions(+), 0 deletions(-)