| Message ID | 1275424897-32253-5-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Luiz Capitulino <lcapitulino@redhat.com> writes: > This commit introduces check_client_args_type(), which is > called by qmp_check_client_args() and complements the > previous commit. > > Now the new client's argument checker code is capable of > doing type checking and detecting unknown arguments. > > It works this way: we iterate over the client's arguments > qdict and for each argument we check if it exists and if > its type is correct. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > monitor.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 76 insertions(+), 1 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 47a0da8..14790e6 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4266,6 +4266,75 @@ typedef struct QMPArgCheckRes { > } QMPArgCheckRes; > > /* > + * Check if client's argument exists and type is correct > + */ > +static void check_client_args_type(const char *client_arg_name, > + QObject *client_arg, void *opaque) > +{ > + QObject *obj; > + QString *arg_type; > + QMPArgCheckRes *res = opaque; > + > + if (res->result < 0) { > + /* report only the first error */ > + return; > + } > + > + obj = qdict_get(res->qdict, client_arg_name); > + if (!obj) { > + /* client arg doesn't exist */ > + res->result = -1; > + qerror_report(QERR_INVALID_PARAMETER, client_arg_name); > + return; > + } > + > + arg_type = qobject_to_qstring(obj); > + assert(arg_type != NULL); > + > + /* check if argument's type is correct */ > + switch (qstring_get_str(arg_type)[0]) { > + case 'F': > + case 'B': > + case 's': > + if (qobject_type(client_arg) != QTYPE_QSTRING) { > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, > + "string"); > + res->result = -1; > + } > + break; > + case 'i': > + case 'l': > + case 'M': > + if (qobject_type(client_arg) != QTYPE_QINT) { > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "int"); > + res->result = -1; > + } > + break; > + case 'f': > + case 'T': > + if (qobject_type(client_arg) != QTYPE_QINT && > + qobject_type(client_arg) != QTYPE_QFLOAT) { > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, > + "number"); > + res->result = -1; > + } > + break; > + case 'b': > + case '-': > + if (qobject_type(client_arg) != QTYPE_QBOOL) { > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "bool"); > + res->result = -1; > + } > + break; > + case 'O': > + /* Not checked here */ > + break; What about case '/'? I guess it doesn't make much sense for QMP, but the old checker handles it. If we drop it from QMP, we should document the restriction in the source. > + default: > + abort(); > + } > +} > + > +/* > * Check if client passed all mandatory args > */ > 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. Again, the iterator makes for tortuous code. > > QDECREF(cmd_args); > return res.result;
On Wed, 02 Jun 2010 09:31:24 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > This commit introduces check_client_args_type(), which is > > called by qmp_check_client_args() and complements the > > previous commit. > > > > Now the new client's argument checker code is capable of > > doing type checking and detecting unknown arguments. > > > > It works this way: we iterate over the client's arguments > > qdict and for each argument we check if it exists and if > > its type is correct. > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > monitor.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 files changed, 76 insertions(+), 1 deletions(-) > > > > diff --git a/monitor.c b/monitor.c > > index 47a0da8..14790e6 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -4266,6 +4266,75 @@ typedef struct QMPArgCheckRes { > > } QMPArgCheckRes; > > > > /* > > + * Check if client's argument exists and type is correct > > + */ > > +static void check_client_args_type(const char *client_arg_name, > > + QObject *client_arg, void *opaque) > > +{ > > + QObject *obj; > > + QString *arg_type; > > + QMPArgCheckRes *res = opaque; > > + > > + if (res->result < 0) { > > + /* report only the first error */ > > + return; > > + } > > + > > + obj = qdict_get(res->qdict, client_arg_name); > > + if (!obj) { > > + /* client arg doesn't exist */ > > + res->result = -1; > > + qerror_report(QERR_INVALID_PARAMETER, client_arg_name); > > + return; > > + } > > + > > + arg_type = qobject_to_qstring(obj); > > + assert(arg_type != NULL); > > + > > + /* check if argument's type is correct */ > > + switch (qstring_get_str(arg_type)[0]) { > > + case 'F': > > + case 'B': > > + case 's': > > + if (qobject_type(client_arg) != QTYPE_QSTRING) { > > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, > > + "string"); > > + res->result = -1; > > + } > > + break; > > + case 'i': > > + case 'l': > > + case 'M': > > + if (qobject_type(client_arg) != QTYPE_QINT) { > > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "int"); > > + res->result = -1; > > + } > > + break; > > + case 'f': > > + case 'T': > > + if (qobject_type(client_arg) != QTYPE_QINT && > > + qobject_type(client_arg) != QTYPE_QFLOAT) { > > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, > > + "number"); > > + res->result = -1; > > + } > > + break; > > + case 'b': > > + case '-': > > + if (qobject_type(client_arg) != QTYPE_QBOOL) { > > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "bool"); > > + res->result = -1; > > + } > > + break; > > + case 'O': > > + /* Not checked here */ > > + break; > > What about case '/'? I guess it doesn't make much sense for QMP, but > the old checker handles it. If we drop it from QMP, we should document > the restriction in the source. Yes, there're two args_type we don't handle in QMP because we don't have any of those handlers converted: '/' and '.'. I think it's unlikely to get them converted in this form, so the current implementation contains dead-code. I explained this in one commit log, but maybe it's the wrong place. > > > + default: > > + abort(); > > + } > > +} > > + > > +/* > > * Check if client passed all mandatory args > > */ > > 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. Oh, good catch. I thought that that was what the current code does, but looks like I misread it. > Again, the iterator makes for tortuous code. > > > > > QDECREF(cmd_args); > > return res.result; >
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 02 Jun 2010 09:31:24 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >> > This commit introduces check_client_args_type(), which is >> > called by qmp_check_client_args() and complements the >> > previous commit. >> > >> > Now the new client's argument checker code is capable of >> > doing type checking and detecting unknown arguments. >> > >> > It works this way: we iterate over the client's arguments >> > qdict and for each argument we check if it exists and if >> > its type is correct. >> > >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >> > --- >> > monitor.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> > 1 files changed, 76 insertions(+), 1 deletions(-) >> > >> > diff --git a/monitor.c b/monitor.c >> > index 47a0da8..14790e6 100644 >> > --- a/monitor.c >> > +++ b/monitor.c >> > @@ -4266,6 +4266,75 @@ typedef struct QMPArgCheckRes { >> > } QMPArgCheckRes; >> > >> > /* >> > + * Check if client's argument exists and type is correct >> > + */ >> > +static void check_client_args_type(const char *client_arg_name, >> > + QObject *client_arg, void *opaque) >> > +{ >> > + QObject *obj; >> > + QString *arg_type; >> > + QMPArgCheckRes *res = opaque; >> > + >> > + if (res->result < 0) { >> > + /* report only the first error */ >> > + return; >> > + } >> > + >> > + obj = qdict_get(res->qdict, client_arg_name); >> > + if (!obj) { >> > + /* client arg doesn't exist */ >> > + res->result = -1; >> > + qerror_report(QERR_INVALID_PARAMETER, client_arg_name); >> > + return; >> > + } >> > + >> > + arg_type = qobject_to_qstring(obj); >> > + assert(arg_type != NULL); >> > + >> > + /* check if argument's type is correct */ >> > + switch (qstring_get_str(arg_type)[0]) { >> > + case 'F': >> > + case 'B': >> > + case 's': >> > + if (qobject_type(client_arg) != QTYPE_QSTRING) { >> > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, >> > + "string"); >> > + res->result = -1; >> > + } >> > + break; >> > + case 'i': >> > + case 'l': >> > + case 'M': >> > + if (qobject_type(client_arg) != QTYPE_QINT) { >> > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "int"); >> > + res->result = -1; >> > + } >> > + break; >> > + case 'f': >> > + case 'T': >> > + if (qobject_type(client_arg) != QTYPE_QINT && >> > + qobject_type(client_arg) != QTYPE_QFLOAT) { >> > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, >> > + "number"); >> > + res->result = -1; >> > + } >> > + break; >> > + case 'b': >> > + case '-': >> > + if (qobject_type(client_arg) != QTYPE_QBOOL) { >> > + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "bool"); >> > + res->result = -1; >> > + } >> > + break; >> > + case 'O': >> > + /* Not checked here */ >> > + break; >> >> What about case '/'? I guess it doesn't make much sense for QMP, but >> the old checker handles it. If we drop it from QMP, we should document >> the restriction in the source. > > Yes, there're two args_type we don't handle in QMP because we don't have > any of those handlers converted: '/' and '.'. > > I think it's unlikely to get them converted in this form, so the current > implementation contains dead-code. I explained this in one commit log, but > maybe it's the wrong place. I read that commit message after I sent my comment :) The commit message is fine; I'd like a comment in the code in addition, not instead of the commit message. [...]
On Wed, 02 Jun 2010 09:31:24 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@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. * Does this work with the current argument checker?
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 02 Jun 2010 09:31:24 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Luiz Capitulino <lcapitulino@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. 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. 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. 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. Complication: you can have a QemuOptsList accept arbitrary arguments, regardless of their name. Checking them is left to its consumer. The way it's currently done (empty desc[] member), such an O-type can't declare *any* names when it accepts arbitrary arguments. Obviously, we can't have more than one O-type parameter accept arbitrary arguments, because we wouldn't know which O-type should get them. An O-type accepting arbitrary arguments should receive the "left-overs", i.e. any arguments that can't be matched to named parameters. Summary: * Parameter names must be unique, whether they are defined directly in args_type or included by an O-type. * We can't have multiple O-types accepting arbitrary arguments. * If we have an O-type accepting arbitrary arguments, it receives any arguments that can't be matched to named parameters. * I'd be fine with the restriction to a single O-type. > * Does this work with the current argument checker? I added O-types in the device_add series. I implemented only what device_add needs, because that series was already awkwardly long. 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.
On Mon, 21 Jun 2010 10:12:06 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Wed, 02 Jun 2010 09:31:24 +0200 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> Luiz Capitulino <lcapitulino@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.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Mon, 21 Jun 2010 10:12:06 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >> > On Wed, 02 Jun 2010 09:31:24 +0200 >> > Markus Armbruster <armbru@redhat.com> wrote: >> > >> >> Luiz Capitulino <lcapitulino@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 "can't have" for crying out loud! >> 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". Workable, I think. > 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. Just like any other type, the O-type tells us how to parse a single positional argument. > 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. Should be good enough for the initial commit.
diff --git a/monitor.c b/monitor.c index 47a0da8..14790e6 100644 --- a/monitor.c +++ b/monitor.c @@ -4266,6 +4266,75 @@ typedef struct QMPArgCheckRes { } QMPArgCheckRes; /* + * Check if client's argument exists and type is correct + */ +static void check_client_args_type(const char *client_arg_name, + QObject *client_arg, void *opaque) +{ + QObject *obj; + QString *arg_type; + QMPArgCheckRes *res = opaque; + + if (res->result < 0) { + /* report only the first error */ + return; + } + + obj = qdict_get(res->qdict, client_arg_name); + if (!obj) { + /* client arg doesn't exist */ + res->result = -1; + qerror_report(QERR_INVALID_PARAMETER, client_arg_name); + return; + } + + arg_type = qobject_to_qstring(obj); + assert(arg_type != NULL); + + /* check if argument's type is correct */ + switch (qstring_get_str(arg_type)[0]) { + case 'F': + case 'B': + case 's': + if (qobject_type(client_arg) != QTYPE_QSTRING) { + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, + "string"); + res->result = -1; + } + break; + case 'i': + case 'l': + case 'M': + if (qobject_type(client_arg) != QTYPE_QINT) { + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "int"); + res->result = -1; + } + break; + case 'f': + case 'T': + if (qobject_type(client_arg) != QTYPE_QINT && + qobject_type(client_arg) != QTYPE_QFLOAT) { + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, + "number"); + res->result = -1; + } + break; + case 'b': + case '-': + if (qobject_type(client_arg) != QTYPE_QBOOL) { + qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "bool"); + res->result = -1; + } + break; + case 'O': + /* Not checked here */ + break; + default: + abort(); + } +} + +/* * Check if client passed all mandatory args */ 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); + } QDECREF(cmd_args); return res.result;
This commit introduces check_client_args_type(), which is called by qmp_check_client_args() and complements the previous commit. Now the new client's argument checker code is capable of doing type checking and detecting unknown arguments. It works this way: we iterate over the client's arguments qdict and for each argument we check if it exists and if its type is correct. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- monitor.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 76 insertions(+), 1 deletions(-)