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

2010-06-03 Thread Markus Armbruster 
Luiz Capitulino lcapitul...@redhat.com writes:

 On Wed, 02 Jun 2010 08:59:11 +0200
 Markus Armbruster arm...@redhat.com wrote:

 Luiz Capitulino lcapitul...@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.

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

2010-06-02 Thread Markus Armbruster 
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;

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);
 +}
 +}
[...]

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

2010-06-02 Thread Markus Armbruster 
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 =

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

2010-06-02 Thread Luiz Capitulino 
On Wed, 02 Jun 2010 08:59:11 +0200
Markus Armbruster arm...@redhat.com wrote:

 Luiz Capitulino lcapitul...@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?

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

2010-06-02 Thread Markus Armbruster 
Luiz Capitulino lcapitul...@redhat.com writes:

 On Wed, 02 Jun 2010 09:22:40 +0200
 Markus Armbruster arm...@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.

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

2010-06-01 Thread Luiz Capitulino 
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;
+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;
-- 
1.7.1.231.gd0b16