Re: [Qemu-devel] Re: [RFC][PATCH 1/2] machine-specific command line switches

[Glauber Costa]

On Mon, May 19, 2008 at 10:53 AM, Jan Kiszka <address@hidden> wrote:
> Glauber Costa wrote:
>> On Sat, May 17, 2008 at 10:34 AM, Jan Kiszka <address@hidden> wrote:
>>> For a different project, I once wrote a patch to organize purely
>>> machine-specific command line switches under the hood of the respective
>>> machine implementations. Now the MusicPal has precisely that need as
>>> well. So I reanimated the patch, and here we go:
>>>
>>> The idea is to add two fields to QEMUMachine and process them:
>>>  o options_help - a string that is inserted under a separate section of
>>>   the "qemu -h" output.
>>>  o parse_option - a callback invoked if a given option was not handled
>>>   by the generic code. It returns -1 if the option is unkown, 0 if it
>>>   is know but comes without an argument, and 1 when the argument was
>>>   consumed.
>>
>> This would be quite useful for QEMUAccel too.
>>
>> So...
>>
>>> +typedef int QEMUMachineParseOption(const char *optname, const char 
>>> *optarg);
>>> +
>>>  typedef struct QEMUMachine {
>>>     const char *name;
>>>     const char *desc;
>>>     QEMUMachineInitFunc *init;
>>>  #define RAMSIZE_FIXED  (1 << 0)
>>>     ram_addr_t ram_require;
>>> +    const char *options_help;
>>> +    QEMUMachineParseOption *parse_option;
>>>     struct QEMUMachine *next;
>>>  } QEMUMachine;
>>
>> Why don't turn the naming into a more generic one? Maybe QEMUOpts, or
>> smth like this.
>>
>>> Index: b/vl.c
>>> ===================================================================
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -7141,6 +7141,8 @@ static int main_loop(void)
>>>
>>>  static void help(int exitcode)
>>>  {
>>> +    QEMUMachine *m;
>>> +
>>>     printf("QEMU PC emulator version " QEMU_VERSION ", Copyright (c) 
>>> 2003-2008 Fabrice Bellard\n"
>>>            "usage: %s [options] [disk_image]\n"
>>>            "\n"
>>> @@ -7275,14 +7277,7 @@ static void help(int exitcode)
>>>            "-clock          force the use of the given methods for timer 
>>> alarm.\n"
>>>            "                To see what timers are available use -clock ?\n"
>>>            "-startdate      select initial date of the clock\n"
>>> -           "\n"
>>> -           "During emulation, the following keys are useful:\n"
>>> -           "ctrl-alt-f      toggle full screen\n"
>>> -           "ctrl-alt-n      switch to virtual console 'n'\n"
>>> -           "ctrl-alt        toggle mouse and keyboard grab\n"
>>> -           "\n"
>>> -           "When using -nographic, press 'ctrl-a h' to get some help.\n"
>>> -           ,
>>> +           "\n",
>>>            "qemu",
>>>            DEFAULT_RAM_SIZE,
>>>  #ifndef _WIN32
>>> @@ -7291,6 +7286,17 @@ static void help(int exitcode)
>>>  #endif
>>>            DEFAULT_GDBSTUB_PORT,
>>>            "/tmp/qemu.log");
>>> +    for (m = first_machine; m != NULL; m = m->next) {
>>> +        if (m->options_help)
>>> +            printf("Options specific to %s machine:\n%s\n",
>>> +                   m->name, m->options_help);
>>> +    }
>> So, If I understand correctly what you mean here, This will print out
>> specific options for every registered machine, right?
>
> Right.
>
>> It does not sound correct, since we won't have support for more than
>> one in the same binary anyway. It looks correct, tough, if we think
>
> I was thinking of such examples:
>
> $ arm-softmmu/qemu-system-arm -M ?
> Supported machines are:
> integratorcp ARM Integrator/CP (ARM926EJ-S) (default)
> versatilepb ARM Versatile/PB (ARM926EJ-S)
> versatileab ARM Versatile/AB (ARM926EJ-S)
> realview   ARM RealView Emulation Baseboard (ARM926EJ-S)
> akita      Akita PDA (PXA270)
> spitz      Spitz PDA (PXA270)
> borzoi     Borzoi PDA (PXA270)
> terrier    Terrier PDA (PXA270)
> cheetah    Palm Tungsten|E aka. Cheetah PDA (OMAP310)
> n800       Nokia N800 tablet aka. RX-34 (OMAP2420)
> n810       Nokia N810 tablet aka. RX-44 (OMAP2420)
> lm3s811evb Stellaris LM3S811EVB
> lm3s6965evb Stellaris LM3S6965EVB
> connex     Gumstix Connex (PXA255)
> verdex     Gumstix Verdex (PXA270)
> mainstone  Mainstone II (PXA27x)
> musicpal   Marvell 88w8618 / MusicPal (ARM926EJ-S)
>
> And only the latter one needs a special switch here. Thus the user
> should know that this switch is not interpreted if some other machine is
> picked.

Now, with the examples it becomes clear. I was messing "machines" and
"architectures" in my head.

>> that we're not representing 'machines', but anything dumping specific
>> options here. But in this case, wouldn't it be better to leave the
>> whole help string
>> to the user of the interface, instead of just using m->name and m->help?
>
> Nevertheless, I do agree that a non-machine oriented abstraction would
> be even nicer in order to organize all those specific options without
> the help of increasing #ifdef'ery. However, yet no smart idea came to my
> mind to handle all cases (per-arch, per-machine, per-accelerator,
> per-whatever - and all this combined).
>

I think that your patch as-is comes very close to this. Just the
naming and a few details need to be changed to make it a generic
interface
for everyone wanting to put specific options in.

>>> +
>>> +            result = -1;
>>> +            for (m = first_machine; m != NULL; m = m->next) {
>>> +                if (m->parse_option) {
>> I don't like this very much. There's no point in having specific
>> options without this parse_option anyway. So it would be better to
>> check for it before displaying the options at all, and simplify the
>> code here.
>
> Don't understand your concern yet. If machine provides options_help, it
> is supposed to provide parse_options and vice versa. Thus you don't dump
> help about non-existent services - unless someone messes up the machine
> definition.

Precisely. Since it is supposed to always happen together, we should
check for them together.
It could look like:

+    for (m = first_machine; m != NULL; m = m->next) {
+        if (m->options_help) && (m->parse_options)
+            printf("Options specific to %s machine:\n%s\n",
+                   m->name, m->options_help);
+    }

in the piece of code above, and so we can simplify it a  little at this point.

>
> OK, let's try again and do some brainstorming about more flexible option
> handling. My initial idea was machine focused (both for the MusicPal and
> here @work), thus I stuffed the information into the machine descriptor.
> However, my scenario would be fine as well when the machines register
> some additional options (in what ever form). The same could be done by
> arch-common initialization functions for per-arch options or by an
> accelerator initializer for its special switches. Sounds feasible?
a lot.



-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."