From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Jy5GC-0008CO-Ek for qemu-devel@nongnu.org; Mon, 19 May 2008 09:18:40 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Jy5GB-0008Br-Od for qemu-devel@nongnu.org; Mon, 19 May 2008 09:18:40 -0400 Received: from [199.232.76.173] (port=57927 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Jy5GB-0008BU-Hw for qemu-devel@nongnu.org; Mon, 19 May 2008 09:18:39 -0400 Received: from wf-out-1314.google.com ([209.85.200.174]:48316) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Jy5GB-00076C-DY for qemu-devel@nongnu.org; Mon, 19 May 2008 09:18:39 -0400 Received: by wf-out-1314.google.com with SMTP id 27so1337801wfd.4 for ; Mon, 19 May 2008 06:18:36 -0700 (PDT) Message-ID: <5d6222a80805190618l59f2e312mfedd96c9ce343652@mail.gmail.com> Date: Mon, 19 May 2008 10:18:36 -0300 From: "Glauber Costa" Subject: Re: [Qemu-devel] [RFC][PATCH 1/2] machine-specific command line switches In-Reply-To: <482EDEF8.7030309@web.de> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <482EDEF8.7030309@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 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? 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 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? > @@ -7673,7 +7679,7 @@ int main(int argc, char **argv) > const char *gdbstub_port; > #endif > uint32_t boot_devices_bitmap = 0; > - int i; > + int i, result; > int snapshot, linux_boot, net_boot; > const char *initrd_filename; > const char *kernel_filename, *kernel_cmdline; > @@ -7692,7 +7698,7 @@ int main(int argc, char **argv) > const char *parallel_devices[MAX_PARALLEL_PORTS]; > int parallel_device_index; > const char *loadvm = NULL; > - QEMUMachine *machine; > + QEMUMachine *machine, *m; > const char *cpu_model; > const char *usb_devices[MAX_USB_CMDLINE]; > int usb_devices_index; > @@ -7784,6 +7790,21 @@ int main(int argc, char **argv) > /* Treat --foo the same as -foo. */ > if (r[1] == '-') > r++; > + > + 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. > + result = m->parse_option(r, > + (optind < argc) ? argv[optind] : NULL); > + if (result >= 0) > + break; > + } > + } > + if (result >= 0) { > + optind += result; > + continue; > + } > + Other than the commented, it all looks very good. -- Glauber Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act."