Re: [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 09 Feb 2012 17:19:00 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 09.02.2012 16:16, schrieb Markus Armbruster:
> >> Kevin Wolf <kwolf@redhat.com> 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 <armbru@redhat.com>
> >>>> ---
> >>>>  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.
>