From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41640) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RvWug-00061L-Cv for qemu-devel@nongnu.org; Thu, 09 Feb 2012 11:32:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RvWua-0004sQ-2f for qemu-devel@nongnu.org; Thu, 09 Feb 2012 11:32:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48146) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RvWuZ-0004sI-Od for qemu-devel@nongnu.org; Thu, 09 Feb 2012 11:31:56 -0500 Date: Thu, 9 Feb 2012 14:31:50 -0200 From: Luiz Capitulino Message-ID: <20120209143150.2608800f@doriath.home> In-Reply-To: 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> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: Markus Armbruster Cc: Kevin Wolf , aliguori@us.ibm.com, qemu-devel@nongnu.org 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. > > 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. >