Patchwork [10/19] qemu-char: Chardev open error reporting, !_WIN32 part

login
register
mail settings
Submitter Markus Armbruster
Date Feb. 7, 2012, 2:09 p.m.
Message ID <1328623766-12287-11-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/139968/
State New
Headers show

Comments

Markus Armbruster - Feb. 7, 2012, 2:09 p.m.
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(-)
Markus Armbruster - Feb. 9, 2012, 3:16 p.m.
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.

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.
Kevin Wolf - Feb. 9, 2012, 3:39 p.m.
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.

> 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
Markus Armbruster - Feb. 9, 2012, 4:19 p.m.
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.

>                                            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.
Luiz Capitulino - Feb. 9, 2012, 4:31 p.m.
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.
>
Markus Armbruster - Feb. 9, 2012, 5:08 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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.

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.

Patch

diff --git a/qemu-char.c b/qemu-char.c
index d591f70..47bab4f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -636,11 +636,18 @@  static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
 
 static CharDriverState *qemu_chr_open_file_out(QemuOpts *opts)
 {
+    const char *filename = qemu_opt_get(opts, "path");
     int fd_out;
 
-    TFR(fd_out = qemu_open(qemu_opt_get(opts, "path"),
-                      O_WRONLY | O_TRUNC | O_CREAT | O_BINARY, 0666));
+    if (!filename) {
+        error_report("file character device requires parameter path");
+        return NULL;
+    }
+
+    TFR(fd_out = qemu_open(filename,
+                           O_WRONLY | O_TRUNC | O_CREAT | O_BINARY, 0666));
     if (fd_out < 0) {
+        error_report("Can't create file '%s': %s", filename, strerror(errno));
         return NULL;
     }
     return qemu_chr_open_fd(-1, fd_out);
@@ -653,7 +660,7 @@  static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
     const char *filename = qemu_opt_get(opts, "path");
 
     if (filename == NULL) {
-        fprintf(stderr, "chardev: pipe: no filename given\n");
+        error_report("pipe character device requires parameter path");
         return NULL;
     }
 
@@ -668,6 +675,8 @@  static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
 	    close(fd_out);
         TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
         if (fd_in < 0) {
+            error_report("Can't create pipe '%s': %s",
+                         filename, strerror(errno));
             return NULL;
         }
     }
@@ -767,6 +776,7 @@  static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
     CharDriverState *chr;
 
     if (stdio_nb_clients >= STDIO_MAX_CLIENTS) {
+        error_report("Too many stdio devices");
         return NULL;
     }
     if (stdio_nb_clients == 0) {
@@ -987,6 +997,7 @@  static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
 #endif
 
     if (openpty(&master_fd, &slave_fd, pty_name, NULL, NULL) < 0) {
+        error_report("Can't open pty: %s", strerror(errno));
         return NULL;
     }
 
@@ -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;