From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35559) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RvXUL-0002ZH-Ri for qemu-devel@nongnu.org; Thu, 09 Feb 2012 12:08:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RvXU9-00041p-Fa for qemu-devel@nongnu.org; Thu, 09 Feb 2012 12:08:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:1625) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RvXU9-00041f-7b for qemu-devel@nongnu.org; Thu, 09 Feb 2012 12:08:41 -0500 From: Markus Armbruster References: <1328623766-12287-1-git-send-email-armbru@redhat.com> <1328623766-12287-11-git-send-email-armbru@redhat.com> <4F3148BC.7000400@redhat.com> <4F33E8A5.1010203@redhat.com> <20120209143150.2608800f@doriath.home> Date: Thu, 09 Feb 2012 18:08:38 +0100 In-Reply-To: <20120209143150.2608800f@doriath.home> (Luiz Capitulino's message of "Thu, 9 Feb 2012 14:31:50 -0200") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Kevin Wolf , aliguori@us.ibm.com, qemu-devel@nongnu.org Luiz Capitulino writes: > On Thu, 09 Feb 2012 17:19:00 +0100 > Markus Armbruster wrote: > >> Kevin Wolf writes: >> >> > Am 09.02.2012 16:16, schrieb Markus Armbruster: >> >> Kevin Wolf writes: >> >> >> >>> Am 07.02.2012 15:09, schrieb Markus Armbruster: >> >>>> This part takes care of backends "file", "pipe", "pty" and "stdio". >> >>>> Unlike many other backends, these leave open error reporting to their >> >>>> caller. Because the caller doesn't know what went wrong, this results >> >>>> in a pretty useless error message. >> >>>> >> >>>> Change them to report their errors. Improves comically user-hostile >> >>>> messages like this one for "-chardev file,id=foo,path=/x" >> >>>> >> >>>> chardev: opening backend "file" failed >> >>>> >> >>>> to >> >>>> >> >>>> qemu-system-x86_64: -chardev file,id=foo,path=/x: Can't create file '/x': Permission denied >> >>>> chardev: opening backend "file" failed >> >>>> >> >>>> The useless "opening backend failed" message will be cleaned up >> >>>> shortly. >> >>>> >> >>>> Signed-off-by: Markus Armbruster >> >>>> --- >> >>>> qemu-char.c | 19 +++++++++++++++---- >> >>>> 1 files changed, 15 insertions(+), 4 deletions(-) >> >>> >> >>>> @@ -1002,7 +1013,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts) >> >>>> chr->filename = g_malloc(len); >> >>>> snprintf(chr->filename, len, "pty:%s", q_ptsname(master_fd)); >> >>>> qemu_opt_set(opts, "path", q_ptsname(master_fd)); >> >>>> - fprintf(stderr, "char device redirected to %s\n", q_ptsname(master_fd)); >> >>>> + error_printf("char device redirected to %s\n", q_ptsname(master_fd)); >> >>>> >> >>>> s = g_malloc0(sizeof(PtyCharDriver)); >> >>>> chr->opaque = s; >> >>> >> >>> Not really an error message. Does it make any sense at all to have this >> >>> message? >> >> >> >> error_printf() prints to the error channel (monitor or stderr), but not >> >> necessarily an error message. See for instance drive_init()'s use of it >> >> to print format help. >> > >> > Ah, right. I confused it with error_report() which includes an error >> > location. That would be rather unexpected. >> >> Indeed. >> >> >> Not sure whether it makes sense to have this message. I guess it exists >> >> because the pty is chosen automatically, but the user might still want >> >> to know which one was chosen. >> > >> > Does "the user" include management tools? >> > >> > If we had a chardev_add monitor command, its output would be moved from >> > stderr to the monitor. We don't have that, >> >> Yet! One of the reasons for doing this series was preparing the ground >> for chardev_add. > > I haven't taken a look in detail in this series, but if your end goal is to > add chardev_add, then you should probably be using error_set() all around. The goal is to improve the atrocious error reporting, no more, no less. Nice side effect: it gets us one little step closer to chardev_add. Converting to error_set() will be much easier after this series than before.