From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46336) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QkfRx-0000FK-Oz for qemu-devel@nongnu.org; Sat, 23 Jul 2011 12:53:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QkfRv-000795-Ew for qemu-devel@nongnu.org; Sat, 23 Jul 2011 12:53:13 -0400 Received: from mail-pz0-f43.google.com ([209.85.210.43]:47867) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QkfRu-000791-WD for qemu-devel@nongnu.org; Sat, 23 Jul 2011 12:53:11 -0400 Received: by pzk1 with SMTP id 1so5809470pzk.30 for ; Sat, 23 Jul 2011 09:53:10 -0700 (PDT) Message-ID: <4E2AFC72.50803@redhat.com> Date: Sat, 23 Jul 2011 11:53:06 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1306927751-8352-1-git-send-email-kwolf@redhat.com> In-Reply-To: <1306927751-8352-1-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qemu-char: Print strerror message on failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org On 06/01/2011 06:29 AM, Kevin Wolf wrote: > The only way for chardev drivers to communicate an error was to return a NULL > pointer, which resulted in an error message that said _that_ something went > wrong, but not _why_. > > This patch changes the interface to return 0/-errno and updates > qemu_chr_open_opts to use strerror to display a more helpful error message. > > Signed-off-by: Kevin Wolf Applied. Thanks. Regards, Anthony Liguori > --- > console.c | 8 ++- > console.h | 2 +- > hw/baum.c | 7 +- > hw/msmouse.c | 5 +- > hw/msmouse.h | 2 +- > qemu-char.c | 165 +++++++++++++++++++++++++++++++---------------------- > spice-qemu-char.c | 9 ++- > ui/qemu-spice.h | 2 +- > 8 files changed, 117 insertions(+), 83 deletions(-) > > diff --git a/console.c b/console.c > index 871c1d4..314d625 100644 > --- a/console.c > +++ b/console.c > @@ -1507,7 +1507,7 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds) > chr->init(chr); > } > > -CharDriverState *text_console_init(QemuOpts *opts) > +int text_console_init(QemuOpts *opts, CharDriverState **_chr) > { > CharDriverState *chr; > TextConsole *s; > @@ -1539,7 +1539,7 @@ CharDriverState *text_console_init(QemuOpts *opts) > > if (!s) { > free(chr); > - return NULL; > + return -EBUSY; > } > > s->chr = chr; > @@ -1547,7 +1547,9 @@ CharDriverState *text_console_init(QemuOpts *opts) > s->g_height = height; > chr->opaque = s; > chr->chr_set_echo = text_console_set_echo; > - return chr; > + > + *_chr = chr; > + return 0; > } > > void text_consoles_set_display(DisplayState *ds) > diff --git a/console.h b/console.h > index 64d1f09..c09537b 100644 > --- a/console.h > +++ b/console.h > @@ -354,7 +354,7 @@ void vga_hw_text_update(console_ch_t *chardata); > > int is_graphic_console(void); > int is_fixedsize_console(void); > -CharDriverState *text_console_init(QemuOpts *opts); > +int text_console_init(QemuOpts *opts, CharDriverState **_chr); > void text_consoles_set_display(DisplayState *ds); > void console_select(unsigned int index); > void console_color_init(DisplayState *ds); > diff --git a/hw/baum.c b/hw/baum.c > index 2aaf5ff..33a22a7 100644 > --- a/hw/baum.c > +++ b/hw/baum.c > @@ -576,7 +576,7 @@ static void baum_close(struct CharDriverState *chr) > qemu_free(baum); > } > > -CharDriverState *chr_baum_init(QemuOpts *opts) > +int chr_baum_init(QemuOpts *opts, CharDriverState **_chr) > { > BaumDriverState *baum; > CharDriverState *chr; > @@ -629,7 +629,8 @@ CharDriverState *chr_baum_init(QemuOpts *opts) > > qemu_chr_generic_open(chr); > > - return chr; > + *_chr = chr; > + return 0; > > fail: > qemu_free_timer(baum->cellCount_timer); > @@ -638,5 +639,5 @@ fail_handle: > qemu_free(handle); > qemu_free(chr); > qemu_free(baum); > - return NULL; > + return -EIO; > } > diff --git a/hw/msmouse.c b/hw/msmouse.c > index 05f893c..67c6cd4 100644 > --- a/hw/msmouse.c > +++ b/hw/msmouse.c > @@ -64,7 +64,7 @@ static void msmouse_chr_close (struct CharDriverState *chr) > qemu_free (chr); > } > > -CharDriverState *qemu_chr_open_msmouse(QemuOpts *opts) > +int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr) > { > CharDriverState *chr; > > @@ -74,5 +74,6 @@ CharDriverState *qemu_chr_open_msmouse(QemuOpts *opts) > > qemu_add_mouse_event_handler(msmouse_event, chr, 0, "QEMU Microsoft Mouse"); > > - return chr; > + *_chr = chr; > + return 0; > } > diff --git a/hw/msmouse.h b/hw/msmouse.h > index 456cb21..8b853b3 100644 > --- a/hw/msmouse.h > +++ b/hw/msmouse.h > @@ -1,2 +1,2 @@ > /* msmouse.c */ > -CharDriverState *qemu_chr_open_msmouse(QemuOpts *opts); > +int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr); > diff --git a/qemu-char.c b/qemu-char.c > index 5e04a20..a8e4094 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -220,13 +220,15 @@ static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len) > return len; > } > > -static CharDriverState *qemu_chr_open_null(QemuOpts *opts) > +static int qemu_chr_open_null(QemuOpts *opts, CharDriverState **_chr) > { > CharDriverState *chr; > > chr = qemu_mallocz(sizeof(CharDriverState)); > chr->chr_write = null_chr_write; > - return chr; > + > + *_chr= chr; > + return 0; > } > > /* MUX driver for serial I/O splitting */ > @@ -635,18 +637,21 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out) > return chr; > } > > -static CharDriverState *qemu_chr_open_file_out(QemuOpts *opts) > +static int qemu_chr_open_file_out(QemuOpts *opts, CharDriverState **_chr) > { > int fd_out; > > TFR(fd_out = qemu_open(qemu_opt_get(opts, "path"), > O_WRONLY | O_TRUNC | O_CREAT | O_BINARY, 0666)); > - if (fd_out< 0) > - return NULL; > - return qemu_chr_open_fd(-1, fd_out); > + if (fd_out< 0) { > + return -errno; > + } > + > + *_chr = qemu_chr_open_fd(-1, fd_out); > + return 0; > } > > -static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts) > +static int qemu_chr_open_pipe(QemuOpts *opts, CharDriverState **_chr) > { > int fd_in, fd_out; > char filename_in[256], filename_out[256]; > @@ -654,7 +659,7 @@ static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts) > > if (filename == NULL) { > fprintf(stderr, "chardev: pipe: no filename given\n"); > - return NULL; > + return -EINVAL; > } > > snprintf(filename_in, 256, "%s.in", filename); > @@ -666,11 +671,14 @@ static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts) > close(fd_in); > if (fd_out>= 0) > close(fd_out); > - TFR(fd_in = fd_out = open(filename, O_RDWR | O_BINARY)); > - if (fd_in< 0) > - return NULL; > + TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY)); > + if (fd_in< 0) { > + return -errno; > + } > } > - return qemu_chr_open_fd(fd_in, fd_out); > + > + *_chr = qemu_chr_open_fd(fd_in, fd_out); > + return 0; > } > > > @@ -761,12 +769,14 @@ static void qemu_chr_close_stdio(struct CharDriverState *chr) > fd_chr_close(chr); > } > > -static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts) > +static int qemu_chr_open_stdio(QemuOpts *opts, CharDriverState **_chr) > { > CharDriverState *chr; > > - if (stdio_nb_clients>= STDIO_MAX_CLIENTS) > - return NULL; > + if (stdio_nb_clients>= STDIO_MAX_CLIENTS) { > + return -EBUSY; > + } > + > if (stdio_nb_clients == 0) { > old_fd0_flags = fcntl(0, F_GETFL); > tcgetattr (0,&oldtty); > @@ -783,7 +793,8 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts) > display_type != DT_NOGRAPHIC); > qemu_chr_set_echo(chr, false); > > - return chr; > + *_chr = chr; > + return 0; > } > > #ifdef __sun__ > @@ -970,7 +981,7 @@ static void pty_chr_close(struct CharDriverState *chr) > qemu_chr_event(chr, CHR_EVENT_CLOSED); > } > > -static CharDriverState *qemu_chr_open_pty(QemuOpts *opts) > +static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr) > { > CharDriverState *chr; > PtyCharDriver *s; > @@ -988,7 +999,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts) > s = qemu_mallocz(sizeof(PtyCharDriver)); > > if (openpty(&s->fd,&slave_fd, pty_name, NULL, NULL)< 0) { > - return NULL; > + return -errno; > } > > /* Set raw attributes on the pty. */ > @@ -1010,7 +1021,8 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts) > > s->timer = qemu_new_timer_ms(rt_clock, pty_chr_timer, chr); > > - return chr; > + *_chr = chr; > + return 0; > } > > static void tty_serial_init(int fd, int speed, > @@ -1211,30 +1223,28 @@ static void qemu_chr_close_tty(CharDriverState *chr) > } > } > > -static CharDriverState *qemu_chr_open_tty(QemuOpts *opts) > +static int qemu_chr_open_tty(QemuOpts *opts, CharDriverState **_chr) > { > const char *filename = qemu_opt_get(opts, "path"); > CharDriverState *chr; > int fd; > > - TFR(fd = open(filename, O_RDWR | O_NONBLOCK)); > + TFR(fd = qemu_open(filename, O_RDWR | O_NONBLOCK)); > if (fd< 0) { > - return NULL; > + return -errno; > } > tty_serial_init(fd, 115200, 'N', 8, 1); > chr = qemu_chr_open_fd(fd, fd); > - if (!chr) { > - close(fd); > - return NULL; > - } > chr->chr_ioctl = tty_serial_ioctl; > chr->chr_close = qemu_chr_close_tty; > - return chr; > + > + *_chr = chr; > + return 0; > } > #else /* ! __linux__&& ! __sun__ */ > -static CharDriverState *qemu_chr_open_pty(QemuOpts *opts) > +static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr) > { > - return NULL; > + return -ENOTSUP; > } > #endif /* __linux__ || __sun__ */ > > @@ -1348,7 +1358,7 @@ static void pp_close(CharDriverState *chr) > qemu_chr_event(chr, CHR_EVENT_CLOSED); > } > > -static CharDriverState *qemu_chr_open_pp(QemuOpts *opts) > +static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr) > { > const char *filename = qemu_opt_get(opts, "path"); > CharDriverState *chr; > @@ -1356,12 +1366,13 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts) > int fd; > > TFR(fd = open(filename, O_RDWR)); > - if (fd< 0) > - return NULL; > + if (fd< 0) { > + return -errno; > + } > > if (ioctl(fd, PPCLAIM)< 0) { > close(fd); > - return NULL; > + return -errno; > } > > drv = qemu_mallocz(sizeof(ParallelCharDriver)); > @@ -1376,7 +1387,8 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts) > > qemu_chr_generic_open(chr); > > - return chr; > + *_chr = chr; > + return 0; > } > #endif /* __linux__ */ > > @@ -1418,21 +1430,24 @@ static int pp_ioctl(CharDriverState *chr, int cmd, void *arg) > return 0; > } > > -static CharDriverState *qemu_chr_open_pp(QemuOpts *opts) > +static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr) > { > const char *filename = qemu_opt_get(opts, "path"); > CharDriverState *chr; > int fd; > > - fd = open(filename, O_RDWR); > - if (fd< 0) > - return NULL; > + fd = qemu_open(filename, O_RDWR); > + if (fd< 0) { > + return -errno; > + } > > chr = qemu_mallocz(sizeof(CharDriverState)); > chr->opaque = (void *)(intptr_t)fd; > chr->chr_write = null_chr_write; > chr->chr_ioctl = pp_ioctl; > - return chr; > + > + *_chr = chr; > + return 0; > } > #endif > > @@ -1638,7 +1653,7 @@ static int win_chr_poll(void *opaque) > return 0; > } > > -static CharDriverState *qemu_chr_open_win(QemuOpts *opts) > +static int qemu_chr_open_win(QemuOpts *opts, CharDriverState **_chr) > { > const char *filename = qemu_opt_get(opts, "path"); > CharDriverState *chr; > @@ -1653,10 +1668,12 @@ static CharDriverState *qemu_chr_open_win(QemuOpts *opts) > if (win_chr_init(chr, filename)< 0) { > free(s); > free(chr); > - return NULL; > + return -EIO; > } > qemu_chr_generic_open(chr); > - return chr; > + > + *_chr = chr; > + return 0; > } > > static int win_chr_pipe_poll(void *opaque) > @@ -1738,7 +1755,7 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename) > } > > > -static CharDriverState *qemu_chr_open_win_pipe(QemuOpts *opts) > +static int qemu_chr_open_win_pipe(QemuOpts *opts, CharDriverState **_chr) > { > const char *filename = qemu_opt_get(opts, "path"); > CharDriverState *chr; > @@ -1753,10 +1770,12 @@ static CharDriverState *qemu_chr_open_win_pipe(QemuOpts *opts) > if (win_chr_pipe_init(chr, filename)< 0) { > free(s); > free(chr); > - return NULL; > + return -EIO; > } > qemu_chr_generic_open(chr); > - return chr; > + > + *_chr = chr; > + return 0; > } > > static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out) > @@ -1773,22 +1792,23 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out) > return chr; > } > > -static CharDriverState *qemu_chr_open_win_con(QemuOpts *opts) > +static int qemu_chr_open_win_con(QemuOpts *opts, CharDriverState **_chr) > { > - return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE)); > + return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE), chr); > } > > -static CharDriverState *qemu_chr_open_win_file_out(QemuOpts *opts) > +static int qemu_chr_open_win_file_out(QemuOpts *opts, CharDriverState **_chr) > { > const char *file_out = qemu_opt_get(opts, "path"); > HANDLE fd_out; > > fd_out = CreateFile(file_out, GENERIC_WRITE, FILE_SHARE_READ, NULL, > OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); > - if (fd_out == INVALID_HANDLE_VALUE) > - return NULL; > + if (fd_out == INVALID_HANDLE_VALUE) { > + return -EIO; > + } > > - return qemu_chr_open_win_file(fd_out); > + return qemu_chr_open_win_file(fd_out, _chr); > } > #endif /* !_WIN32 */ > > @@ -1869,11 +1889,12 @@ static void udp_chr_close(CharDriverState *chr) > qemu_chr_event(chr, CHR_EVENT_CLOSED); > } > > -static CharDriverState *qemu_chr_open_udp(QemuOpts *opts) > +static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr) > { > CharDriverState *chr = NULL; > NetCharDriver *s = NULL; > int fd = -1; > + int ret; > > chr = qemu_mallocz(sizeof(CharDriverState)); > s = qemu_mallocz(sizeof(NetCharDriver)); > @@ -1881,6 +1902,7 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts) > fd = inet_dgram_opts(opts); > if (fd< 0) { > fprintf(stderr, "inet_dgram_opts failed\n"); > + ret = -errno; > goto return_err; > } > > @@ -1891,16 +1913,17 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts) > chr->chr_write = udp_chr_write; > chr->chr_update_read_handler = udp_chr_update_read_handler; > chr->chr_close = udp_chr_close; > - return chr; > + > + *_chr = chr; > + return 0; > > return_err: > - if (chr) > - free(chr); > - if (s) > - free(s); > - if (fd>= 0) > + qemu_free(chr); > + qemu_free(s); > + if (fd>= 0) { > closesocket(fd); > - return NULL; > + } > + return ret; > } > > /***********************************************************/ > @@ -2179,7 +2202,7 @@ static void tcp_chr_close(CharDriverState *chr) > qemu_chr_event(chr, CHR_EVENT_CLOSED); > } > > -static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) > +static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr) > { > CharDriverState *chr = NULL; > TCPCharDriver *s = NULL; > @@ -2189,6 +2212,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) > int do_nodelay; > int is_unix; > int is_telnet; > + int ret; > > is_listen = qemu_opt_get_bool(opts, "server", 0); > is_waitconnect = qemu_opt_get_bool(opts, "wait", 1); > @@ -2214,8 +2238,10 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) > fd = inet_connect_opts(opts); > } > } > - if (fd< 0) > + if (fd< 0) { > + ret = -errno; > goto fail; > + } > > if (!is_waitconnect) > socket_set_nonblock(fd); > @@ -2267,14 +2293,16 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) > tcp_chr_accept(chr); > socket_set_nonblock(s->listen_fd); > } > - return chr; > + > + *_chr = chr; > + return 0; > > fail: > if (fd>= 0) > closesocket(fd); > qemu_free(s); > qemu_free(chr); > - return NULL; > + return ret; > } > > /***********************************************************/ > @@ -2467,7 +2495,7 @@ fail: > > static const struct { > const char *name; > - CharDriverState *(*open)(QemuOpts *opts); > + int (*open)(QemuOpts *opts, CharDriverState **chr); > } backend_table[] = { > { .name = "null", .open = qemu_chr_open_null }, > { .name = "socket", .open = qemu_chr_open_socket }, > @@ -2507,6 +2535,7 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts, > { > CharDriverState *chr; > int i; > + int ret; > > if (qemu_opts_id(opts) == NULL) { > fprintf(stderr, "chardev: no id specified\n"); > @@ -2528,10 +2557,10 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts, > return NULL; > } > > - chr = backend_table[i].open(opts); > - if (!chr) { > - fprintf(stderr, "chardev: opening backend \"%s\" failed\n", > - qemu_opt_get(opts, "backend")); > + ret = backend_table[i].open(opts,&chr); > + if (ret< 0) { > + fprintf(stderr, "chardev: opening backend \"%s\" failed: %s\n", > + qemu_opt_get(opts, "backend"), strerror(-ret)); > return NULL; > } > > diff --git a/spice-qemu-char.c b/spice-qemu-char.c > index fa15a71..bf004c3 100644 > --- a/spice-qemu-char.c > +++ b/spice-qemu-char.c > @@ -160,7 +160,7 @@ static void print_allowed_subtypes(void) > fprintf(stderr, "\n"); > } > > -CharDriverState *qemu_chr_open_spice(QemuOpts *opts) > +int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr) > { > CharDriverState *chr; > SpiceCharDriver *s; > @@ -172,7 +172,7 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts) > if (name == NULL) { > fprintf(stderr, "spice-qemu-char: missing name parameter\n"); > print_allowed_subtypes(); > - return NULL; > + return -EINVAL; > } > for(;*psubtype != NULL; ++psubtype) { > if (strcmp(name, *psubtype) == 0) { > @@ -183,7 +183,7 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts) > if (subtype == NULL) { > fprintf(stderr, "spice-qemu-char: unsupported name\n"); > print_allowed_subtypes(); > - return NULL; > + return -EINVAL; > } > > chr = qemu_mallocz(sizeof(CharDriverState)); > @@ -200,5 +200,6 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts) > > qemu_chr_generic_open(chr); > > - return chr; > + *_chr = chr; > + return 0; > } > diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h > index 3c6f1fe..f34be69 100644 > --- a/ui/qemu-spice.h > +++ b/ui/qemu-spice.h > @@ -42,7 +42,7 @@ int qemu_spice_migrate_info(const char *hostname, int port, int tls_port, > void do_info_spice_print(Monitor *mon, const QObject *data); > void do_info_spice(Monitor *mon, QObject **ret_data); > > -CharDriverState *qemu_chr_open_spice(QemuOpts *opts); > +int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr); > > #else /* CONFIG_SPICE */ >