Re: [Qemu-devel] [PATCH 5/5] vnc: No need for Error** parameter at vnc_client_io_error()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2150 bytes --]

On 06/08/2017 08:39 AM, Eduardo Habkost wrote:
> The only callers of vnc_client_io_error() with non-NULL errp don't use
> *errp after the function gets called, so there's no need to use an
> Error** argument.  Change parameter to Error* and simplify the code.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  ui/vnc.h |  2 +-
>  ui/vnc.c | 13 +++++--------
>  2 files changed, 6 insertions(+), 9 deletions(-)

>  
> -ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
> +ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
>  {

This is unusual.

>      if (ret <= 0) {
>          if (ret == 0) {
> @@ -1188,14 +1188,11 @@ ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>              vnc_disconnect_start(vs);
>          } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
>              VNC_DEBUG("Closing down client sock: ret %zd (%s)\n",
> -                      ret, errp ? error_get_pretty(*errp) : "Unknown");
> +                      ret, err ? error_get_pretty(err) : "Unknown");
>              vnc_disconnect_start(vs);
>          }
>  
> -        if (errp) {
> -            error_free(*errp);
> -            *errp = NULL;
> -        }
> +        error_free(err);

Worse, it's action at a distance. You are freeing memory here that was
allocated in the callers.

>          return 0;
>      }
>      return ret;
> @@ -1231,7 +1228,7 @@ ssize_t vnc_client_write_buf(VncState *vs, const uint8_t *data, size_t datalen)
>      ret = qio_channel_write(
>          vs->ioc, (const char *)data, datalen, &err);
>      VNC_DEBUG("Wrote wire %p %zd -> %ld\n", data, datalen, ret);
> -    return vnc_client_io_error(vs, ret, &err);
> +    return vnc_client_io_error(vs, ret, err);
>  }

It looks like the only reason that err gets passed is for the sake of
VNC_DEBUG messages, but I'm not sure I like this patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]