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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Thu, 09 Feb 2012 17:19:00 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Kevin Wolf <address@hidden> writes:
>> 
>> > Am 09.02.2012 16:16, schrieb Markus Armbruster:
>> >> Kevin Wolf <address@hidden> 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 <address@hidden>
>> >>>> ---
>> >>>>  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.