Patchwork [06/19] Add qemu_opts_validate() for post parsing validation

login
register
mail settings
Submitter Mark McLoughlin
Date Sept. 10, 2009, 3:18 p.m.
Message ID <1252595941-15196-7-git-send-email-markmc@redhat.com>
Download mbox | patch
Permalink /patch/33330/
State Superseded
Headers show

Comments

Mark McLoughlin - Sept. 10, 2009, 3:18 p.m.
Several qemu command line options have a parameter whose value affects
what other parameters are accepted for the option.

In these cases, we can have an empty description table in the
QemuOptsList and once the option has been parsed we can use a suitable
description table to validate the other parameters based on the value of
that parameter.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu-option.c |   32 ++++++++++++++++++++++++++++++++
 qemu-option.h |    1 +
 2 files changed, 33 insertions(+), 0 deletions(-)
Gerd Hoffmann - Sept. 11, 2009, 7:47 a.m.
On 09/10/09 17:18, Mark McLoughlin wrote:
> Several qemu command line options have a parameter whose value affects
> what other parameters are accepted for the option.
>
> In these cases, we can have an empty description table in the
> QemuOptsList and once the option has been parsed we can use a suitable
> description table to validate the other parameters based on the value of
> that parameter.

Hmm.  The "empty description" mode is intended to be used in case the 
verification isn't done by QemuOpts at all (i.e. for -devices, where the 
qdev property code will do the checks).  I really like to have the 
option description hooked into QemuOptsList.

I can think of two possible ways to do that:

(1) We can stick all possible values info QemuOptsList->desc.  Then
     have separate data structures to describe which fields are allowed
     in which cases (and, while being at it, which fields are mandatory).

(2) We have multiple QemuOptDesc lists for the different cases.

I tend to prefer the first.  We can do the option parsing 
unconditionally in qemu_opt_set() then and don't have two different 
possible code paths for that.

Comments?  Other ideas?

cheers,
   Gerd
Mark McLoughlin - Sept. 11, 2009, 12:38 p.m.
On Fri, 2009-09-11 at 09:47 +0200, Gerd Hoffmann wrote:
> On 09/10/09 17:18, Mark McLoughlin wrote:
> > Several qemu command line options have a parameter whose value affects
> > what other parameters are accepted for the option.
> >
> > In these cases, we can have an empty description table in the
> > QemuOptsList and once the option has been parsed we can use a suitable
> > description table to validate the other parameters based on the value of
> > that parameter.
> 
> Hmm.  The "empty description" mode is intended to be used in case the 
> verification isn't done by QemuOpts at all (i.e. for -devices, where the 
> qdev property code will do the checks).  I really like to have the 
> option description hooked into QemuOptsList.
> 
> I can think of two possible ways to do that:
> 
> (1) We can stick all possible values info QemuOptsList->desc.  Then
>      have separate data structures to describe which fields are allowed
>      in which cases (and, while being at it, which fields are mandatory).

You'd need to make it part of the QemuOptDesc to make it easy to figure
out from reading the code which parameters apply to which types.

e.g. a QemuOptDesc::firstname_values field

> (2) We have multiple QemuOptDesc lists for the different cases.

As with what I did, you'll have some parameters which are common to
different types.

> I tend to prefer the first.  We can do the option parsing 
> unconditionally in qemu_opt_set() then and don't have two different 
> possible code paths for that.

Sounds reasonable, except it is nice that right now we have the
parameter descriptions associated with, and close to, the init function.

Cheers,
Mark.
Gerd Hoffmann - Sept. 11, 2009, 1:51 p.m.
On 09/11/09 14:38, Mark McLoughlin wrote:
> On Fri, 2009-09-11 at 09:47 +0200, Gerd Hoffmann wrote:
>> (1) We can stick all possible values info QemuOptsList->desc.  Then
>>       have separate data structures to describe which fields are allowed
>>       in which cases (and, while being at it, which fields are mandatory).
>
> You'd need to make it part of the QemuOptDesc to make it easy to figure
> out from reading the code which parameters apply to which types.
>
> e.g. a QemuOptDesc::firstname_values field

Yes, that would work as well.

>> (2) We have multiple QemuOptDesc lists for the different cases.
>
> As with what I did, you'll have some parameters which are common to
> different types.

One of the reasons I'd prefer (1).

cheers,
   Gerd

Patch

diff --git a/qemu-option.c b/qemu-option.c
index d312257..0f517d1 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -777,6 +777,38 @@  QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, const char *fi
     return opts;
 }
 
+/* Validate parsed opts against descriptions where no
+ * descriptions were provided in the QemuOptsList.
+ */
+int qemu_opts_validate(QemuOpts *opts, QemuOptDesc *desc)
+{
+    QemuOpt *opt;
+
+    assert(opts->list->desc[0].name == NULL);
+
+    TAILQ_FOREACH(opt, &opts->head, next) {
+        int i;
+
+        for (i = 0; desc[i].name != NULL; i++) {
+            if (strcmp(desc[i].name, opt->name) == 0) {
+                break;
+            }
+        }
+        if (desc[i].name == NULL) {
+            fprintf(stderr, "option \"%s\" is not valid for %s\n",
+                    opt->name, opts->list->name);
+            return -1;
+        }
+
+        opt->desc = &desc[i];
+
+        if (qemu_opt_parse(opt) < 0)
+            return -1;
+    }
+
+    return 0;
+}
+
 int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
                       int abort_on_failure)
 {
diff --git a/qemu-option.h b/qemu-option.h
index 9e52625..e816d95 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -114,6 +114,7 @@  int qemu_opts_set(QemuOptsList *list, const char *id,
                   const char *name, const char *value);
 const char *qemu_opts_id(QemuOpts *opts);
 void qemu_opts_del(QemuOpts *opts);
+int qemu_opts_validate(QemuOpts *opts, QemuOptDesc *desc);
 int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname);
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, const char *firstname);