From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53221) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RvWiG-00024v-1z for qemu-devel@nongnu.org; Thu, 09 Feb 2012 11:19:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RvWi6-0002Xs-Rp for qemu-devel@nongnu.org; Thu, 09 Feb 2012 11:19:11 -0500 Received: from mx1.redhat.com ([209.132.183.28]:8482) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RvWi6-0002XK-JP for qemu-devel@nongnu.org; Thu, 09 Feb 2012 11:19:02 -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> Date: Thu, 09 Feb 2012 17:19:00 +0100 In-Reply-To: <4F33E8A5.1010203@redhat.com> (Kevin Wolf's message of "Thu, 09 Feb 2012 16:39:17 +0100") 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: Kevin Wolf Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, Luiz Capitulino 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. > but there might be commands > that create chardevs internally: gdbserver is one, not sure if I missed > others. > > We probably don't care much about breaking tools that use 'gdbserver > pty' and read the device from stderr. And do so using a monitor chardev other than stdio. That would be weird, wouldn't it? > (But the information is missing > from QMP - should it be added?) Right when somebody asks for it. For what it's worth, some chardev backend open() methods return such information via their opts argument. E.g. inet_listen_opts() receives a port range in opts (options "port" and "to"), and overwrites option "port" with the port actually chosen. I hate that, should use separate options.