From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Jy5wm-0002L1-Gr for qemu-devel@nongnu.org; Mon, 19 May 2008 10:02:40 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Jy5wl-0002Ki-Sj for qemu-devel@nongnu.org; Mon, 19 May 2008 10:02:40 -0400 Received: from [199.232.76.173] (port=37774 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Jy5wl-0002Kc-Dj for qemu-devel@nongnu.org; Mon, 19 May 2008 10:02:39 -0400 Received: from yw-out-1718.google.com ([74.125.46.154]:35255) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Jy5wl-0000Qh-0s for qemu-devel@nongnu.org; Mon, 19 May 2008 10:02:39 -0400 Received: by yw-out-1718.google.com with SMTP id 6so1177895ywa.82 for ; Mon, 19 May 2008 07:02:36 -0700 (PDT) Message-ID: <5d6222a80805190702p762cd67uf789366238b9fda7@mail.gmail.com> Date: Mon, 19 May 2008 11:02:35 -0300 From: "Glauber Costa" Subject: Re: [Qemu-devel] Re: [RFC][PATCH 1/2] machine-specific command line switches In-Reply-To: <48318641.800@siemens.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <482EDEF8.7030309@web.de> <5d6222a80805190618l59f2e312mfedd96c9ce343652@mail.gmail.com> <48318641.800@siemens.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 On Mon, May 19, 2008 at 10:53 AM, Jan Kiszka wrote: > Glauber Costa wrote: >> On Sat, May 17, 2008 at 10:34 AM, Jan Kiszka 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."