Re: [Qemu-devel] QEMU configuration files

[Jamie Lokier <jamie@shareable.org>]

Ian Jackson wrote:
> 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.

Yes, if it can be tidy, clear and compact.

> 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:
> 
> +	    { "cyls", qemu_fieldcheck_int,  .min=1, .max=16383 }

Yes.

> 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:
...
> 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 */
> +       };

Yes again.

I do something very similar for options processing in some of my
programs recently, and it's been nice to use.  It removes a lot of
duplicated boilerplate, like the switch statements and so on.

I go a little further, letting FIELD_SIGNED work on any signed integer
(it's got sizeof()) and FIELD_UNSIGNED, FIELD_STRING, and convenient
specials like FIELD_HOSTNAME_OR_IP.  It's working out nicely so far.

You can also add help text to each option, if you have some pretty way
to dump all the option names and their help texts.  I'm finding it
tidier than maintaining separate documentation strings, and easier to
keep it correct.

GNU argp (now in Glibc) has some similar ideas.  It doesn't let the
options point directly to where to store values.  It's quite good for
help strings and such.

While we're here, it would be nice if QEMU's monitor could use this
generic config code to show all the config settings, using the same
names as the config file - and let you change all the settings too,
when that's possible.

-- Jamie