From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KBEX8-0005wW-Eq for qemu-devel@nongnu.org; Tue, 24 Jun 2008 15:50:30 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KBEX8-0005wK-51 for qemu-devel@nongnu.org; Tue, 24 Jun 2008 15:50:30 -0400 Received: from [199.232.76.173] (port=42672 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KBEX7-0005wD-SA for qemu-devel@nongnu.org; Tue, 24 Jun 2008 15:50:29 -0400 Received: from mail2.shareable.org ([80.68.89.115]:52259) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1KBEX7-0001zm-4f for qemu-devel@nongnu.org; Tue, 24 Jun 2008 15:50:29 -0400 Received: from jamie by mail2.shareable.org with local (Exim 4.63) (envelope-from ) id 1KBEX3-0005eT-VX for qemu-devel@nongnu.org; Tue, 24 Jun 2008 20:50:26 +0100 Date: Tue, 24 Jun 2008 20:50:25 +0100 From: Jamie Lokier Subject: Re: [Qemu-devel] QEMU configuration files Message-ID: <20080624195025.GB21292@shareable.org> References: <48595024.7050400@bellard.org> <18529.6059.522984.377076@mariner.uk.xensource.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <18529.6059.522984.377076@mariner.uk.xensource.com> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.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