[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] Modular command line options
From: |
Glauber Costa |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] Modular command line options |
Date: |
Wed, 21 May 2008 16:17:48 -0300 |
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."