Re: [Qemu-devel] [PATCH 01/19] Revert "qemu-char: Print strerror message on failure" and deps

[Anthony Liguori <aliguori@us.ibm.com>]

On 02/07/2012 08:09 AM, Markus Armbruster wrote:
> The commit's purpose is laudable:
>
>      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_.
>
> It attempts to achieve it by changing the interface to return 0/-errno
> and update qemu_chr_open_opts() to use strerror() to display a more
> helpful error message.  Unfortunately, it has serious flaws:
>
> 1. Backends "socket" and "udp" return bogus error codes, because
> qemu_chr_open_socket() and qemu_chr_open_udp() assume that
> unix_listen_opts(), unix_connect_opts(), inet_listen_opts(),
> inet_connect_opts() and inet_dgram_opts() fail with errno set
> appropriately.  That assumption is wrong, and the commit turns
> unspecific error messages into misleading error messages.  For
> instance:
>
>      $ qemu-system-x86_64 -nodefaults -vnc :0 -chardev socket,id=bar,host=xxx
>      inet_connect: host and/or port not specified
>      chardev: opening backend "socket" failed: No such file or directory
>
> ENOENT is what happens to be in my errno when the backend returns
> -errno.  Let's put ERANGE there just for giggles:
>
>      $ qemu-system-x86_64 -nodefaults -vnc :0 -chardev socket,id=bar,host=xxx -drive if=none,iops=99999999999999999999
>      inet_connect: host and/or port not specified
>      chardev: opening backend "socket" failed: Numerical result out of range
>
> Worse: when errno happens to be zero, return -errno erroneously
> signals success, and qemu_chr_new_from_opts() dies dereferencing
> uninitialized chr.  I observe this with "-serial unix:".
>
> 2. All qemu_chr_open_opts() knows about the error is an errno error
> code.  That's simply not enough for a decent message.  For instance,
> when inet_dgram() can't resolve the parameter host, which errno code
> should it use?  What if it can't resolve parameter localaddr?
>
> Clue: many backends already report errors in their open methods.
> Let's revert the flawed commit along with its dependencies, and fix up
> the silent error paths instead.
>
> This reverts commit 6e1db57b2ac9025c2443c665a0d9e78748637b26.

Ack on this, but see comments in other patches for what I think we should do 
instead.

Regards,

Anthony Liguori

>
> Conflicts:
>
> 	console.c
> 	hw/baum.c
> 	qemu-char.c
>
> This reverts commit aad04cd024f0c59f0b96f032cde2e24eb3abba6d.
>
> The parts of commit db418a0a "Add stdio char device on windows" that
> depend on the reverted change fixed up.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> ---
>   console.c         |    8 +--
>   console.h         |    2 +-
>   hw/baum.c         |    7 +-
>   hw/baum.h         |    2 +-
>   hw/msmouse.c      |    5 +-
>   hw/msmouse.h      |    2 +-
>   qemu-char.c       |  169 +++++++++++++++++++++-------------------------------
>   spice-qemu-char.c |    9 +--
>   ui/qemu-spice.h   |    2 +-
>   9 files changed, 84 insertions(+), 122 deletions(-)
>
> diff --git a/console.c b/console.c
> index 135394f..2c432e3 100644
> --- a/console.c
> +++ b/console.c
> @@ -1510,7 +1510,7 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
>           chr->init(chr);
>   }
>
> -int text_console_init(QemuOpts *opts, CharDriverState **_chr)
> +CharDriverState *text_console_init(QemuOpts *opts)
>   {
>       CharDriverState *chr;
>       TextConsole *s;
> @@ -1542,7 +1542,7 @@ int text_console_init(QemuOpts *opts, CharDriverState **_chr)
>
>       if (!s) {
>           g_free(chr);
> -        return -EBUSY;
> +        return NULL;
>       }
>
>       s->chr = chr;
> @@ -1550,9 +1550,7 @@ int text_console_init(QemuOpts *opts, CharDriverState **_chr)
>       s->g_height = height;
>       chr->opaque = s;
>       chr->chr_set_echo = text_console_set_echo;
> -
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>   }
>
>   void text_consoles_set_display(DisplayState *ds)
> diff --git a/console.h b/console.h
> index 6ba0d5d..c33ffe0 100644
> --- a/console.h
> +++ b/console.h
> @@ -356,7 +356,7 @@ void vga_hw_text_update(console_ch_t *chardata);
>
>   int is_graphic_console(void);
>   int is_fixedsize_console(void);
> -int text_console_init(QemuOpts *opts, CharDriverState **_chr);
> +CharDriverState *text_console_init(QemuOpts *opts);
>   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 86d780a..3e94f84 100644
> --- a/hw/baum.c
> +++ b/hw/baum.c
> @@ -562,7 +562,7 @@ static void baum_close(struct CharDriverState *chr)
>       g_free(baum);
>   }
>
> -int chr_baum_init(QemuOpts *opts, CharDriverState **_chr)
> +CharDriverState *chr_baum_init(QemuOpts *opts)
>   {
>       BaumDriverState *baum;
>       CharDriverState *chr;
> @@ -614,8 +614,7 @@ int chr_baum_init(QemuOpts *opts, CharDriverState **_chr)
>
>       qemu_chr_generic_open(chr);
>
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>
>   fail:
>       qemu_free_timer(baum->cellCount_timer);
> @@ -624,5 +623,5 @@ fail_handle:
>       g_free(handle);
>       g_free(chr);
>       g_free(baum);
> -    return -EIO;
> +    return NULL;
>   }
> diff --git a/hw/baum.h b/hw/baum.h
> index 3f28cc3..8af710f 100644
> --- a/hw/baum.h
> +++ b/hw/baum.h
> @@ -23,4 +23,4 @@
>    */
>
>   /* char device */
> -int chr_baum_init(QemuOpts *opts, CharDriverState **_chr);
> +CharDriverState *chr_baum_init(QemuOpts *opts);
> diff --git a/hw/msmouse.c b/hw/msmouse.c
> index c3b57ea..9c492a4 100644
> --- a/hw/msmouse.c
> +++ b/hw/msmouse.c
> @@ -64,7 +64,7 @@ static void msmouse_chr_close (struct CharDriverState *chr)
>       g_free (chr);
>   }
>
> -int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr)
> +CharDriverState *qemu_chr_open_msmouse(QemuOpts *opts)
>   {
>       CharDriverState *chr;
>
> @@ -74,6 +74,5 @@ int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr)
>
>       qemu_add_mouse_event_handler(msmouse_event, chr, 0, "QEMU Microsoft Mouse");
>
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>   }
> diff --git a/hw/msmouse.h b/hw/msmouse.h
> index 8b853b3..456cb21 100644
> --- a/hw/msmouse.h
> +++ b/hw/msmouse.h
> @@ -1,2 +1,2 @@
>   /* msmouse.c */
> -int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr);
> +CharDriverState *qemu_chr_open_msmouse(QemuOpts *opts);
> diff --git a/qemu-char.c b/qemu-char.c
> index b1d80dd..1e882cf 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -218,15 +218,13 @@ static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>       return len;
>   }
>
> -static int qemu_chr_open_null(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_null(QemuOpts *opts)
>   {
>       CharDriverState *chr;
>
>       chr = g_malloc0(sizeof(CharDriverState));
>       chr->chr_write = null_chr_write;
> -
> -    *_chr= chr;
> -    return 0;
> +    return chr;
>   }
>
>   /* MUX driver for serial I/O splitting */
> @@ -636,21 +634,18 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
>       return chr;
>   }
>
> -static int qemu_chr_open_file_out(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_file_out(QemuOpts *opts)
>   {
>       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 -errno;
> -    }
> -
> -    *_chr = qemu_chr_open_fd(-1, fd_out);
> -    return 0;
> +    if (fd_out<  0)
> +        return NULL;
> +    return qemu_chr_open_fd(-1, fd_out);
>   }
>
> -static int qemu_chr_open_pipe(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
>   {
>       int fd_in, fd_out;
>       char filename_in[256], filename_out[256];
> @@ -658,7 +653,7 @@ static int qemu_chr_open_pipe(QemuOpts *opts, CharDriverState **_chr)
>
>       if (filename == NULL) {
>           fprintf(stderr, "chardev: pipe: no filename given\n");
> -        return -EINVAL;
> +        return NULL;
>       }
>
>       snprintf(filename_in, 256, "%s.in", filename);
> @@ -670,14 +665,11 @@ static int qemu_chr_open_pipe(QemuOpts *opts, CharDriverState **_chr)
>   	    close(fd_in);
>   	if (fd_out>= 0)
>   	    close(fd_out);
> -        TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
> -        if (fd_in<  0) {
> -            return -errno;
> -        }
> +        TFR(fd_in = fd_out = open(filename, O_RDWR | O_BINARY));
> +        if (fd_in<  0)
> +            return NULL;
>       }
> -
> -    *_chr = qemu_chr_open_fd(fd_in, fd_out);
> -    return 0;
> +    return qemu_chr_open_fd(fd_in, fd_out);
>   }
>
>
> @@ -768,14 +760,12 @@ static void qemu_chr_close_stdio(struct CharDriverState *chr)
>       fd_chr_close(chr);
>   }
>
> -static int qemu_chr_open_stdio(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
>   {
>       CharDriverState *chr;
>
> -    if (stdio_nb_clients>= STDIO_MAX_CLIENTS) {
> -        return -EBUSY;
> -    }
> -
> +    if (stdio_nb_clients>= STDIO_MAX_CLIENTS)
> +        return NULL;
>       if (stdio_nb_clients == 0) {
>           old_fd0_flags = fcntl(0, F_GETFL);
>           tcgetattr (0,&oldtty);
> @@ -792,8 +782,7 @@ static int qemu_chr_open_stdio(QemuOpts *opts, CharDriverState **_chr)
>                                              display_type != DT_NOGRAPHIC);
>       qemu_chr_fe_set_echo(chr, false);
>
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>   }
>
>   #ifdef __sun__
> @@ -980,7 +969,7 @@ static void pty_chr_close(struct CharDriverState *chr)
>       qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>   }
>
> -static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
>   {
>       CharDriverState *chr;
>       PtyCharDriver *s;
> @@ -995,7 +984,7 @@ static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
>   #endif
>
>       if (openpty(&master_fd,&slave_fd, pty_name, NULL, NULL)<  0) {
> -        return -errno;
> +        return NULL;
>       }
>
>       /* Set raw attributes on the pty. */
> @@ -1021,8 +1010,7 @@ static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
>       s->fd = master_fd;
>       s->timer = qemu_new_timer_ms(rt_clock, pty_chr_timer, chr);
>
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>   }
>
>   static void tty_serial_init(int fd, int speed,
> @@ -1223,28 +1211,30 @@ static void qemu_chr_close_tty(CharDriverState *chr)
>       }
>   }
>
> -static int qemu_chr_open_tty(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
>   {
>       const char *filename = qemu_opt_get(opts, "path");
>       CharDriverState *chr;
>       int fd;
>
> -    TFR(fd = qemu_open(filename, O_RDWR | O_NONBLOCK));
> +    TFR(fd = open(filename, O_RDWR | O_NONBLOCK));
>       if (fd<  0) {
> -        return -errno;
> +        return NULL;
>       }
>       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;
> -
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>   }
>   #else  /* ! __linux__&&  ! __sun__ */
> -static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
>   {
> -    return -ENOTSUP;
> +    return NULL;
>   }
>   #endif /* __linux__ || __sun__ */
>
> @@ -1358,7 +1348,7 @@ static void pp_close(CharDriverState *chr)
>       qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>   }
>
> -static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
>   {
>       const char *filename = qemu_opt_get(opts, "path");
>       CharDriverState *chr;
> @@ -1366,13 +1356,12 @@ static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
>       int fd;
>
>       TFR(fd = open(filename, O_RDWR));
> -    if (fd<  0) {
> -        return -errno;
> -    }
> +    if (fd<  0)
> +        return NULL;
>
>       if (ioctl(fd, PPCLAIM)<  0) {
>           close(fd);
> -        return -errno;
> +        return NULL;
>       }
>
>       drv = g_malloc0(sizeof(ParallelCharDriver));
> @@ -1387,8 +1376,7 @@ static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
>
>       qemu_chr_generic_open(chr);
>
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>   }
>   #endif /* __linux__ */
>
> @@ -1430,24 +1418,21 @@ static int pp_ioctl(CharDriverState *chr, int cmd, void *arg)
>       return 0;
>   }
>
> -static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
>   {
>       const char *filename = qemu_opt_get(opts, "path");
>       CharDriverState *chr;
>       int fd;
>
> -    fd = qemu_open(filename, O_RDWR);
> -    if (fd<  0) {
> -        return -errno;
> -    }
> +    fd = open(filename, O_RDWR);
> +    if (fd<  0)
> +        return NULL;
>
>       chr = g_malloc0(sizeof(CharDriverState));
>       chr->opaque = (void *)(intptr_t)fd;
>       chr->chr_write = null_chr_write;
>       chr->chr_ioctl = pp_ioctl;
> -
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>   }
>   #endif
>
> @@ -1663,7 +1648,7 @@ static int win_chr_poll(void *opaque)
>       return 0;
>   }
>
> -static int qemu_chr_open_win(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
>   {
>       const char *filename = qemu_opt_get(opts, "path");
>       CharDriverState *chr;
> @@ -1678,12 +1663,10 @@ static int qemu_chr_open_win(QemuOpts *opts, CharDriverState **_chr)
>       if (win_chr_init(chr, filename)<  0) {
>           g_free(s);
>           g_free(chr);
> -        return -EIO;
> +        return NULL;
>       }
>       qemu_chr_generic_open(chr);
> -
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>   }
>
>   static int win_chr_pipe_poll(void *opaque)
> @@ -1765,7 +1748,7 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
>   }
>
>
> -static int qemu_chr_open_win_pipe(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_win_pipe(QemuOpts *opts)
>   {
>       const char *filename = qemu_opt_get(opts, "path");
>       CharDriverState *chr;
> @@ -1780,15 +1763,13 @@ static int qemu_chr_open_win_pipe(QemuOpts *opts, CharDriverState **_chr)
>       if (win_chr_pipe_init(chr, filename)<  0) {
>           g_free(s);
>           g_free(chr);
> -        return -EIO;
> +        return NULL;
>       }
>       qemu_chr_generic_open(chr);
> -
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>   }
>
> -static int qemu_chr_open_win_file(HANDLE fd_out, CharDriverState **pchr)
> +static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
>   {
>       CharDriverState *chr;
>       WinCharState *s;
> @@ -1799,27 +1780,25 @@ static int qemu_chr_open_win_file(HANDLE fd_out, CharDriverState **pchr)
>       chr->opaque = s;
>       chr->chr_write = win_chr_write;
>       qemu_chr_generic_open(chr);
> -    *pchr = chr;
> -    return 0;
> +    return chr;
>   }
>
> -static int qemu_chr_open_win_con(QemuOpts *opts, CharDriverState **chr)
> +static CharDriverState *qemu_chr_open_win_con(QemuOpts *opts)
>   {
> -    return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE), chr);
> +    return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE));
>   }
>
> -static int qemu_chr_open_win_file_out(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_win_file_out(QemuOpts *opts)
>   {
>       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 -EIO;
> -    }
> +    if (fd_out == INVALID_HANDLE_VALUE)
> +        return NULL;
>
> -    return qemu_chr_open_win_file(fd_out, _chr);
> +    return qemu_chr_open_win_file(fd_out);
>   }
>
>   static int win_stdio_write(CharDriverState *chr, const uint8_t *buf, int len)
> @@ -1960,7 +1939,7 @@ static void win_stdio_close(CharDriverState *chr)
>       stdio_nb_clients--;
>   }
>
> -static int qemu_chr_open_win_stdio(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_win_stdio(QemuOpts *opts)
>   {
>       CharDriverState   *chr;
>       WinStdioCharState *stdio;
> @@ -1969,7 +1948,7 @@ static int qemu_chr_open_win_stdio(QemuOpts *opts, CharDriverState **_chr)
>
>       if (stdio_nb_clients>= STDIO_MAX_CLIENTS
>           || ((display_type != DT_NOGRAPHIC)&&  (stdio_nb_clients != 0))) {
> -        return -EIO;
> +        return NULL;
>       }
>
>       chr   = g_malloc0(sizeof(CharDriverState));
> @@ -2028,9 +2007,7 @@ static int qemu_chr_open_win_stdio(QemuOpts *opts, CharDriverState **_chr)
>       chr->chr_set_echo = qemu_chr_set_echo_win_stdio;
>       qemu_chr_fe_set_echo(chr, false);
>
> -    *_chr = chr;
> -
> -    return 0;
> +    return chr;
>   }
>   #endif /* !_WIN32 */
>
> @@ -2111,12 +2088,11 @@ static void udp_chr_close(CharDriverState *chr)
>       qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>   }
>
> -static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
>   {
>       CharDriverState *chr = NULL;
>       NetCharDriver *s = NULL;
>       int fd = -1;
> -    int ret;
>
>       chr = g_malloc0(sizeof(CharDriverState));
>       s = g_malloc0(sizeof(NetCharDriver));
> @@ -2124,7 +2100,6 @@ static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr)
>       fd = inet_dgram_opts(opts);
>       if (fd<  0) {
>           fprintf(stderr, "inet_dgram_opts failed\n");
> -        ret = -errno;
>           goto return_err;
>       }
>
> @@ -2135,9 +2110,7 @@ static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr)
>       chr->chr_write = udp_chr_write;
>       chr->chr_update_read_handler = udp_chr_update_read_handler;
>       chr->chr_close = udp_chr_close;
> -
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>
>   return_err:
>       g_free(chr);
> @@ -2145,7 +2118,7 @@ return_err:
>       if (fd>= 0) {
>           closesocket(fd);
>       }
> -    return ret;
> +    return NULL;
>   }
>
>   /***********************************************************/
> @@ -2436,7 +2409,7 @@ static void tcp_chr_close(CharDriverState *chr)
>       qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>   }
>
> -static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>   {
>       CharDriverState *chr = NULL;
>       TCPCharDriver *s = NULL;
> @@ -2446,7 +2419,6 @@ static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
>       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);
> @@ -2472,10 +2444,8 @@ static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
>               fd = inet_connect_opts(opts);
>           }
>       }
> -    if (fd<  0) {
> -        ret = -errno;
> +    if (fd<  0)
>           goto fail;
> -    }
>
>       if (!is_waitconnect)
>           socket_set_nonblock(fd);
> @@ -2528,16 +2498,14 @@ static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
>           tcp_chr_accept(chr);
>           socket_set_nonblock(s->listen_fd);
>       }
> -
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>
>    fail:
>       if (fd>= 0)
>           closesocket(fd);
>       g_free(s);
>       g_free(chr);
> -    return ret;
> +    return NULL;
>   }
>
>   /***********************************************************/
> @@ -2730,7 +2698,7 @@ fail:
>
>   static const struct {
>       const char *name;
> -    int (*open)(QemuOpts *opts, CharDriverState **chr);
> +    CharDriverState *(*open)(QemuOpts *opts);
>   } backend_table[] = {
>       { .name = "null",      .open = qemu_chr_open_null },
>       { .name = "socket",    .open = qemu_chr_open_socket },
> @@ -2771,7 +2739,6 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>   {
>       CharDriverState *chr;
>       int i;
> -    int ret;
>
>       if (qemu_opts_id(opts) == NULL) {
>           fprintf(stderr, "chardev: no id specified\n");
> @@ -2793,10 +2760,10 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>           return NULL;
>       }
>
> -    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));
> +    chr = backend_table[i].open(opts);
> +    if (!chr) {
> +        fprintf(stderr, "chardev: opening backend \"%s\" failed\n",
> +                qemu_opt_get(opts, "backend"));
>           return NULL;
>       }
>
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> index 7e8eaa9..1e735ff 100644
> --- a/spice-qemu-char.c
> +++ b/spice-qemu-char.c
> @@ -188,7 +188,7 @@ static void print_allowed_subtypes(void)
>       fprintf(stderr, "\n");
>   }
>
> -int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
> +CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
>   {
>       CharDriverState *chr;
>       SpiceCharDriver *s;
> @@ -200,7 +200,7 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
>       if (name == NULL) {
>           fprintf(stderr, "spice-qemu-char: missing name parameter\n");
>           print_allowed_subtypes();
> -        return -EINVAL;
> +        return NULL;
>       }
>       for(;*psubtype != NULL; ++psubtype) {
>           if (strcmp(name, *psubtype) == 0) {
> @@ -211,7 +211,7 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
>       if (subtype == NULL) {
>           fprintf(stderr, "spice-qemu-char: unsupported name\n");
>           print_allowed_subtypes();
> -        return -EINVAL;
> +        return NULL;
>       }
>
>       chr = g_malloc0(sizeof(CharDriverState));
> @@ -233,6 +233,5 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
>       }
>   #endif
>
> -    *_chr = chr;
> -    return 0;
> +    return chr;
>   }
> diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
> index c35b29c..a6c4a2c 100644
> --- a/ui/qemu-spice.h
> +++ b/ui/qemu-spice.h
> @@ -44,7 +44,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);
>
> -int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr);
> +CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
>
>   #else  /* CONFIG_SPICE */
>   #include "monitor.h"