Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 4/5] qemu-char: convert some open functions to use Error API

[Alex Bennée]

zhanghailiang <address@hidden> writes:

> Convert several Character backend open functions to use the Error API.
>
> Signed-off-by: zhanghailiang <address@hidden>
> ---
>  qemu-char.c | 76 
> +++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 41 insertions(+), 35 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 0f38cdd..a1d25c7 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1077,7 +1077,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int 
> fd_out)
>      return chr;
>  }
>  
> -static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
> +static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts, Error 
> **errp)
>  {
>      int fd_in, fd_out;
>      char filename_in[CHR_MAX_FILENAME_SIZE];
<snip>
Why convert the call if we are not using the passed parameter?

>  
> -static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
> +static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts, Error **errp)
>  {
>      CharDriverState *chr;
>  
>      if (is_daemonized()) {
> -        error_report("cannot use stdio with -daemonize");
> +        error_setg(errp, "cannot use stdio with -daemonize");
>          return NULL;
>      }
>  
> @@ -1861,7 +1861,8 @@ static void win_chr_close(CharDriverState *chr)
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>  }
>  
> -static int win_chr_init(CharDriverState *chr, const char *filename)
> +static void win_chr_init(CharDriverState *chr, const char *filename,
> +                         Error **errp)
>  {
>      WinCharState *s = chr->opaque;
>      COMMCONFIG comcfg;
> @@ -1872,25 +1873,25 @@ static int win_chr_init(CharDriverState *chr, const 
> char *filename)
>  
>      s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hsend) {
> -        fprintf(stderr, "CreateEvent failed\n");
> +        error_setg(errp, "CreateEvent failed");
>          goto fail;
>      }
>      s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hrecv) {
> -        fprintf(stderr, "CreateEvent failed\n");
> +        error_setg(errp, "CreateEvent failed");
>          goto fail;
>      }
>  
>      s->hcom = CreateFile(filename, GENERIC_READ|GENERIC_WRITE, 0, NULL,
>                        OPEN_EXISTING, FILE_FLAG_OVERLAPPED, 0);
>      if (s->hcom == INVALID_HANDLE_VALUE) {
> -        fprintf(stderr, "CreateFile (%lu) failed\n", GetLastError());
> +        error_setg(errp, "CreateFile (%lu) failed", GetLastError());
>          s->hcom = NULL;
>          goto fail;
>      }
>  
>      if (!SetupComm(s->hcom, NRECVBUF, NSENDBUF)) {
> -        fprintf(stderr, "SetupComm failed\n");
> +        error_setg(errp, "SetupComm failed");
>          goto fail;
>      }
>  
> @@ -1901,31 +1902,30 @@ static int win_chr_init(CharDriverState *chr, const 
> char *filename)
>      CommConfigDialog(filename, NULL, &comcfg);
>  
>      if (!SetCommState(s->hcom, &comcfg.dcb)) {
> -        fprintf(stderr, "SetCommState failed\n");
> +        error_setg(errp, "SetCommState failed");
>          goto fail;
>      }
>  
>      if (!SetCommMask(s->hcom, EV_ERR)) {
> -        fprintf(stderr, "SetCommMask failed\n");
> +        error_setg(errp, "SetCommMask failed");
>          goto fail;
>      }
>  
>      cto.ReadIntervalTimeout = MAXDWORD;
>      if (!SetCommTimeouts(s->hcom, &cto)) {
> -        fprintf(stderr, "SetCommTimeouts failed\n");
> +        error_setg(errp, "SetCommTimeouts failed");
>          goto fail;
>      }
>  
>      if (!ClearCommError(s->hcom, &err, &comstat)) {
> -        fprintf(stderr, "ClearCommError failed\n");
> +        error_setg(errp, "ClearCommError failed");
>          goto fail;
>      }
>      qemu_add_polling_cb(win_chr_poll, chr);
> -    return 0;
> +    return;
>  
>   fail:
>      win_chr_close(chr);
> -    return -1;
>  }
>  
>  /* Called with chr_write_lock held.  */
> @@ -2022,10 +2022,12 @@ static int win_chr_poll(void *opaque)
>      return 0;
>  }
>  
> -static CharDriverState *qemu_chr_open_win_path(const char *filename)
> +static CharDriverState *qemu_chr_open_win_path(const char *filename,
> +                                               Error **errp)
>  {
>      CharDriverState *chr;
>      WinCharState *s;
> +    Error *local_err = NULL;
>  
>      chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(WinCharState));
> @@ -2033,9 +2035,11 @@ static CharDriverState *qemu_chr_open_win_path(const 
> char *filename)
>      chr->chr_write = win_chr_write;
>      chr->chr_close = win_chr_close;
>  
> -    if (win_chr_init(chr, filename) < 0) {
> +    win_chr_init(chr, filename, &local_err);
> +    if (local_err) {
>          g_free(s);
>          g_free(chr);
> +        error_propagate(errp, local_err);
>          return NULL;

Hmm I'm not sure I find the change from a return value to
pass-by-reference return value intuitive. What does this gain us?

Are the messages now being reported actually more suitable for user
consumption? For example "ClearCommError failed" doesn't actually tell
the user much apart from something went wrong.

>      }
>      return chr;
> @@ -2057,11 +2061,11 @@ static int win_chr_pipe_poll(void *opaque)
>      return 0;
>  }
>  
> -static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
> +static void win_chr_pipe_init(CharDriverState *chr, const char *filename,
> +                              Error **errp)
>  {
>      WinCharState *s = chr->opaque;
>      OVERLAPPED ov;
> -    int ret;
>      DWORD size;
>      char openname[CHR_MAX_FILENAME_SIZE];
>  
> @@ -2069,12 +2073,12 @@ static int win_chr_pipe_init(CharDriverState *chr, 
> const char *filename)
>  
>      s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hsend) {
> -        fprintf(stderr, "CreateEvent failed\n");
> +        error_setg(errp, "CreateEvent failed");
>          goto fail;
>      }
>      s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hrecv) {
> -        fprintf(stderr, "CreateEvent failed\n");
> +        error_setg(errp, "CreateEvent failed");
>          goto fail;
>      }
>  
> @@ -2084,7 +2088,7 @@ static int win_chr_pipe_init(CharDriverState *chr, 
> const char *filename)
>                                PIPE_WAIT,
>                                MAXCONNECT, NSENDBUF, NRECVBUF, NTIMEOUT, 
> NULL);
>      if (s->hcom == INVALID_HANDLE_VALUE) {
> -        fprintf(stderr, "CreateNamedPipe (%lu) failed\n", GetLastError());
> +        error_setg(errp, "CreateNamedPipe (%lu) failed", GetLastError());
>          s->hcom = NULL;
>          goto fail;
>      }
> @@ -2093,13 +2097,13 @@ static int win_chr_pipe_init(CharDriverState *chr, 
> const char *filename)
>      ov.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
>      ret = ConnectNamedPipe(s->hcom, &ov);
>      if (ret) {
> -        fprintf(stderr, "ConnectNamedPipe failed\n");
> +        error_setg(errp, "ConnectNamedPipe failed");
>          goto fail;
>      }
>  
>      ret = GetOverlappedResult(s->hcom, &ov, &size, TRUE);
>      if (!ret) {
> -        fprintf(stderr, "GetOverlappedResult failed\n");
> +        error_setg(errp, "GetOverlappedResult failed");
>          if (ov.hEvent) {
>              CloseHandle(ov.hEvent);
>              ov.hEvent = NULL;
> @@ -2112,19 +2116,19 @@ static int win_chr_pipe_init(CharDriverState *chr, 
> const char *filename)
>          ov.hEvent = NULL;
>      }
>      qemu_add_polling_cb(win_chr_pipe_poll, chr);
> -    return 0;
> +    return;
>  
>   fail:
>      win_chr_close(chr);
> -    return -1;
>  }
>  
>  
> -static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
> +static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts, Error 
> **errp)
>  {
>      const char *filename = opts->device;
>      CharDriverState *chr;
>      WinCharState *s;
> +    Error *local_err = NULL;
>  
>      chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(WinCharState));
> @@ -2132,9 +2136,11 @@ static CharDriverState 
> *qemu_chr_open_pipe(ChardevHostdev *opts)
>      chr->chr_write = win_chr_write;
>      chr->chr_close = win_chr_close;
>  
> -    if (win_chr_pipe_init(chr, filename) < 0) {
> +    win_chr_pipe_init(chr, filename, &local_err);
> +    if (local_err) {
>          g_free(s);
>          g_free(chr);
> +        error_propagate(errp, local_err);
>          return NULL;
>      }
>      return chr;
> @@ -2294,7 +2300,7 @@ static void win_stdio_close(CharDriverState *chr)
>      g_free(chr);
>  }

Same comments as above.

>  
> -static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
> +static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts, Error **errp)
>  {
>      CharDriverState   *chr;
>      WinStdioCharState *stdio;
> @@ -2306,7 +2312,7 @@ static CharDriverState 
> *qemu_chr_open_stdio(ChardevStdio *opts)
>  
>      stdio->hStdIn = GetStdHandle(STD_INPUT_HANDLE);
>      if (stdio->hStdIn == INVALID_HANDLE_VALUE) {
> -        fprintf(stderr, "cannot open stdio: invalid handle\n");
> +        error_report(errp, "cannot open stdio: invalid handle");
>          exit(1);
>      }
>  
> @@ -2319,7 +2325,7 @@ static CharDriverState 
> *qemu_chr_open_stdio(ChardevStdio *opts)
>      if (is_console) {
>          if (qemu_add_wait_object(stdio->hStdIn,
>                                   win_stdio_wait_func, chr)) {
> -            fprintf(stderr, "qemu_add_wait_object: failed\n");
> +            error_setg(errp, "qemu_add_wait_object: failed");
>          }
>      } else {
>          DWORD   dwId;
> @@ -2332,12 +2338,12 @@ static CharDriverState 
> *qemu_chr_open_stdio(ChardevStdio *opts)
>          if (stdio->hInputThread == INVALID_HANDLE_VALUE
>              || stdio->hInputReadyEvent == INVALID_HANDLE_VALUE
>              || stdio->hInputDoneEvent == INVALID_HANDLE_VALUE) {
> -            fprintf(stderr, "cannot create stdio thread or event\n");
> +            error_report(errp, "cannot create stdio thread or event");
>              exit(1);
>          }
>          if (qemu_add_wait_object(stdio->hInputReadyEvent,
>                                   win_stdio_thread_wait_func, chr)) {
> -            fprintf(stderr, "qemu_add_wait_object: failed\n");
> +            error_setg(errp, "qemu_add_wait_object: failed");
>          }
>      }
>  
> @@ -3994,7 +4000,7 @@ static CharDriverState 
> *qmp_chardev_open_file(ChardevFile *file, Error **errp)
>  static CharDriverState *qmp_chardev_open_serial(ChardevHostdev *serial,
>                                                  Error **errp)
>  {
> -    return qemu_chr_open_win_path(serial->device);
> +    return qemu_chr_open_win_path(serial->device, errp);
>  }
>  
>  static CharDriverState *qmp_chardev_open_parallel(ChardevHostdev *parallel,
> @@ -4204,7 +4210,7 @@ ChardevReturn *qmp_chardev_add(const char *id, 
> ChardevBackend *backend,
>          chr = qmp_chardev_open_parallel(backend->parallel, errp);
>          break;
>      case CHARDEV_BACKEND_KIND_PIPE:
> -        chr = qemu_chr_open_pipe(backend->pipe);
> +        chr = qemu_chr_open_pipe(backend->pipe, errp);
>          break;
>      case CHARDEV_BACKEND_KIND_SOCKET:
>          chr = qmp_chardev_open_socket(backend->socket, errp);
> @@ -4241,7 +4247,7 @@ ChardevReturn *qmp_chardev_add(const char *id, 
> ChardevBackend *backend,
>          chr = chr_testdev_init();
>          break;
>      case CHARDEV_BACKEND_KIND_STDIO:
> -        chr = qemu_chr_open_stdio(backend->stdio);
> +        chr = qemu_chr_open_stdio(backend->stdio, errp);
>          break;
>  #ifdef _WIN32
>      case CHARDEV_BACKEND_KIND_CONSOLE:

-- 
Alex Bennée