From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Jytya-0008CF-VM for qemu-devel@nongnu.org; Wed, 21 May 2008 15:27:53 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1JytyY-0008Ad-Qi for qemu-devel@nongnu.org; Wed, 21 May 2008 15:27:52 -0400 Received: from [199.232.76.173] (port=56787 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JytyY-0008AR-EN for qemu-devel@nongnu.org; Wed, 21 May 2008 15:27:50 -0400 Received: from rn-out-0910.google.com ([64.233.170.186]:6965) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1JytyY-0004lB-1H for qemu-devel@nongnu.org; Wed, 21 May 2008 15:27:50 -0400 Received: by rn-out-0910.google.com with SMTP id j71so392799rne.8 for ; Wed, 21 May 2008 12:27:49 -0700 (PDT) Message-ID: <5d6222a80805211217w7c9de193g33ebbae74db81629@mail.gmail.com> Date: Wed, 21 May 2008 16:17:48 -0300 From: "Glauber Costa" Subject: Re: [Qemu-devel] [PATCH 1/3] Modular command line options In-Reply-To: <48336413.2060909@web.de> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <48336413.2060909@web.de> 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 Looks awesome. But if you allow me to be a bit boring again, I have some extra comments. > > +#define HAS_ARG 0x0001 > + > +typedef struct QEMUOption { > + const char *name; > + int flags; > + int index; > +} QEMUOption; > + > +typedef void QEMUOptionParser(int index, const char *optarg); > + > +typedef struct QEMUOptionSet { > + const QEMUOption *options; > + const char *help_string; > + QEMUOptionParser *parse_handler; > + struct QEMUOptionSet *next; > +} QEMUOptionSet; This is a bit counter-intuitive. If we switch to this model, the Help string would be better with the option itself, rather than the option set. Listing the options would then be a matter of iterating over each option, and then printing each of them. An optional help string in the option set could be used as a header. Like this: Help string for the option set: -foo help for foo option, in a QEMUoption -bar help for bar option, in a QEMUOption This seems cleaner and more flexible to me. What do you think? > #ifdef TARGET_PPC > -#define DEFAULT_RAM_SIZE 144 > +#define DEFAULT_RAM_SIZE 144 > +#define DEFAULT_RAM_SIZE_STR "144" > #else > -#define DEFAULT_RAM_SIZE 128 > +#define DEFAULT_RAM_SIZE 128 > +#define DEFAULT_RAM_SIZE_STR "128" > #endif This is a bit ugly IMHO. Defining: __STR(x) #x STR(x) __STR(x) in a common header sounds better. We can then just STR(DEFAULT_RAM_SIZE), or whatever, in any string pasting. Since we're moving to this scheme, it is possible that new needs for doing the same thing will arise, and then just coming up with a bunch of FOO , FOO_STR binoms seems not elegant enough. > +static int snapshot; > -static int drive_init(struct drive_opt *arg, int snapshot, > +static int drive_init(struct drive_opt *arg, int use_snapshot, > QEMUMachine *machine) static global variable, static function. Can't we just not use the argument at all? Yeah, I know it was there before, but... > + > +#ifdef CONFIG_GUS > + { > + "gus", > + "Gravis Ultrasound GF1", > + 0, > + 1, > + { .init_isa = GUS_init } > + }, > #endif For options that are dependant on specific defines, it is better to make them separate, and then register the option set conditionally. But yeah, I know you're not doing everything in this patch. Just wanted to leave the opinion, and see whether or not you agree with it. Other than that, seems all beautiful. -- Glauber Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act."