Re: [Qemu-devel] [PATCH 1/3] Modular command line options

["Glauber Costa" <glommer@gmail.com>]

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."