| Message ID | 1328623766-12287-11-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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. >
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.
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;
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(-)