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

[Kevin Wolf]

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.

> 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, 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. (But the information is missing
from QMP - should it be added?)

Kevin