Re: [Qemu-devel] QEMU configuration files

[Ian Jackson]

Fabrice Bellard writes ("[Qemu-devel] QEMU configuration files"):
> My snapshot of the "object based" QEMU configuration system can be found 
> at http://bellard.org/qemu/patches . I only tried it for x86 targets. It 
> is not yet in committable state and comments are welcome !

This looks like good stuff.

I do have some comments on the structure, though.  Sorry that these
come so late but as you can see I wanted to do a thorough job.  If you
like some of these ideas I'd be happy to provide a concrete patch.

So:


I think it would be better to have a declarative static data structure
describing the options, than to have the data structure which
describes the options constructed dynamically.  This generally
involves less code in the end, and also makes it much easier to do
some of the other things I'm about to suggest.

So for example:

+   void isa_ne2000_class_init(void)
+   {
+       static const QEMUDeviceField fields[]= {
+           { "iobase", QEMU_CTYPE_INT },
+           { "irq",    QEMU_CTYPE_STR },
+           { 0 } /* sentinel */
+       };
+       static const QEMUDeviceClass class= {
+           "ne2k_isa", "NE2000 ISA", fields,
+           .dev_init = isa_ne2000_dev_init
+       };
+       qemu_class_register(&class);
+   }

This is the way the block backend drivers generally already work.

In the rare cases where it is actually necessary to vary the details
at runtime, it is of course still possible to allocate these
structures with malloc and fill them in.  My suggestion is that making
the structure transparent will avoid one level of tail-chasing when
figuring out what's going on, and make it easier to push functionality
out of the individual driver models etc. into the common option
parsing code.


Your version defers checking of the supplied values until the point of
use.  For example, the values of "cyls" etc. for disks are
range-checked in drive_init.  This is less than ideal; generally it's
better to check when the value is first read (from the command line or
config file).  This can greatly improve the error reporting, and can
avoid going through a lot of needless startup work (which may even
need to be undone) if the invocation is doomed.

So I would generally suggest providing the generic parsing code with
not just a type, but also enough information to rangecheck or fully
syntax check the provided value.  In the general case this would be a
function pointer which would use some values (such as the minimum and
maximum values, or other information) which were specified in the same
place as the option.

So I would suggest something like this, instead:

+   void qemu_fieldcheck_int(const QEMUDeviceField*,
+                            const char *value);
+
+   void drive_class_register(void)
+   {
+       static const QEMUDeviceField fields[]= {
+            ...
+           { "cyls", qemu_fieldcheck_int,  .min=1, .max=16383 }
+            ...
+           { 0 } /* sentinel */


Finally, your arrangements mean that each user of a configuration
option to has to call qemu_device_get_* and then store the value of
the option in its own data structure.  This is because the values are
stored, as parsed, indexed by the option name as a string.  I think
that call is an unnecessary and unhelpful indirection; it means we
have to write a lot of boilerplate but doesn't really buy us anything.

I would suggest instead that we have the option parser store the
option values directly into the class's struct, at parse time.  We can
do this by supplying the parser, in the option table, with the offset
of the actual field in the class specific struct:

+   typedef struct NE2000State {
+       ...
+       int irq_option, iobase_option;
+       ...
+   } NE2000State;
+
+   void isa_ne2000_class_init(void)
+   {
+       static const QEMUDeviceField fields[]= {
+           { "iobase", offsetof(NE2000State,iobase_option),
+                        qemu_store_int, .default_int=0, max=INT_MAX },
+           { "irq",    offsetof(NE2000State,irq_option),
+                        qemu_store_irq, .default_int=0 }
+           { 0 } /* sentinel */

This means that at the point of use we can completely eliminate
code like this, because the functionality is in qemu_store_int:

-       cyls = qemu_device_get_int(dev, "cyls");
-       ...
-       if (cyls || heads || secs) {
-           if (cyls < 1 || cyls > 16383) {
-               fprintf(stderr, "qemu: '%d' invalid physical cyls number\n", 
cyls);
-               return -1;
-           }

Doing this also makes it easy to provide field name aliases: we just
add a duplicate entry to the table, pointing at the same option struct
member.  Field name aliases will be needed for backward compatibility
the first time we want to change a field name, and can be useful for
brevity.


This is somewhat clumsy but is much better with some use of macros:

+   void isa_ne2000_class_init(void)
+   {
+       typedef NE2000State ClassState;
+       static const QEMUDeviceField fields[]= {
+           { FIELD_INT(iobase, 0), .max=INT_MAX },
+           { FIELD_INT(irq,    0)               },
+           { 0 } /* sentinel */
+       };

The required macros are something like this:

+   #define FIELD_INT(name,default)             \
+       #name,                                  \
+       offsetof(ClassState, name##_option),    \
+       qemu_store_int,                         \
+       .default_int = (default)
+
+   #define FIELD_IRQ(name,default)             \
+       #name,                                  \
+       offsetof(ClassState, name##_option),    \
+       qemu_store_irq,                         \
+       .default_int = (default)

The types would look something like this:

+   typedef void QEMUStoreDeviceFieldFunction(
+        const struct QEMUDeviceField *field_info,
+        const char *fieldname, const char *value
+       );
+
+   extern QEMUStoreDeviceFieldFunction
+       qemu_store_int, qemu_store_str, qemu_store_irq;
+
+   typedef struct QEMUDeviceField {
+       const char *name;
+       ptrdiff_t store_offset;
+       QEMUStoreDeviceFieldFunction *parser_function;
+       /* remaining fields are used by specific parsers: */
+       int default_int, min, max;
+       const char *default_str;
+   } QEMUDeviceField;


Regards,
Ian.