From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57156 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Og0xn-0001Di-Dz for qemu-devel@nongnu.org; Mon, 02 Aug 2010 15:46:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Og0xl-0002Mb-TI for qemu-devel@nongnu.org; Mon, 02 Aug 2010 15:46:19 -0400 Received: from moutng.kundenserver.de ([212.227.17.8]:63666) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Og0xl-0002Lr-Fs for qemu-devel@nongnu.org; Mon, 02 Aug 2010 15:46:17 -0400 Message-ID: <4C572079.3090209@mail.berlios.de> Date: Mon, 02 Aug 2010 21:46:01 +0200 From: Stefan Weil MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message References: <1280658182-4165-1-git-send-email-weil@mail.berlios.de> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Anthony Liguori , QEMU Developers Am 02.08.2010 10:40, schrieb Markus Armbruster: > Stefan Weil writes: > >> All other error messages in qemu-option.c display the name >> of the invalid parameter. This seems to be reasonable for >> invalid identifiers, too. Without it, a debugger is needed >> to find the name. >> >> Cc: Markus Armbruster >> Cc: Anthony Liguori >> Signed-off-by: Stefan Weil >> --- >> qemu-option.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/qemu-option.c b/qemu-option.c >> index 1f8f41a..ccea267 100644 >> --- a/qemu-option.c >> +++ b/qemu-option.c >> @@ -694,7 +694,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, >> const char *id, int fail_if_exist >> >> if (id) { >> if (!id_wellformed(id)) { >> - qerror_report(QERR_INVALID_PARAMETER_VALUE, "id", "an identifier"); >> + qerror_report(QERR_INVALID_PARAMETER_VALUE, id, "an identifier"); >> error_printf_unless_qmp("Identifiers consist of letters, digits, '-', >> '.', '_', starting with a letter.\n"); >> return NULL; >> } > > No. > > QERR_INVALID_PARAMETER_VALUE's first argument is the parameter *name*, > not the parameter *value*. > > In this case, the parameter name is "id". The variable id contains the > parameter value. > > If you need a debugger to find the offending id=, then location > information is lacking. Could you give an example where it's hard to > find the offending parameter? > > Here's an example where it's easy to find: > > $ qemu-system-x86_64 -device e1000,id=. > qemu-system-x86_64: -device e1000,id=.: Parameter 'id' expects an > identifier > Identifiers consist of letters, digits, '-', '.', '_', starting with a > letter. I had a call of qemu_chr_open with an id containing a space (which looks better in the console display where the id is shown) in my ar7 emulation. You can test this scenario using this patch: --- a/vl.c +++ b/vl.c @@ -1700,7 +1700,7 @@ static int serial_parse(const char *devname) fprintf(stderr, "qemu: too many serial ports\n"); exit(1); } - snprintf(label, sizeof(label), "serial%d", index); + snprintf(label, sizeof(label), "serial %d", index); serial_hds[index] = qemu_chr_open(label, devname, NULL); if (!serial_hds[index]) { fprintf(stderr, "qemu: could not open serial device '%s': %s\n", It results in these error messages: qemu: Parameter 'id' expects an identifier Identifiers consist of letters, digits, '-', '.', '_', starting with a letter. qemu: could not open serial device 'vc:80Cx24C': No such file or directory The "location information" is indeed missing. For this kind of error, an assertion would be better than an error message.