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

[Eduardo Habkost]

On Thu, Jun 08, 2017 at 10:17:45AM -0500, Eric Blake wrote:
> 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 <address@hidden>
> > Cc: Daniel P. Berrange <address@hidden>
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> >  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.

Why?  I would say that using Error** for input (and not output)
is the unusual pattern.

This is the only remaining place in the whole tree where code
assigns something to *errp outside util/error.c (and no callers
of this function rely on this feature).



> 
> >      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.

Isn't this one of the purposes of this function?

The difference here is that now the function function is just
taking ownership of err, making the interface and the
implementation simpler.  If I document this more clearly at
vnc_client_io_error()'s declaration, would it make this change
acceptable?

> 
> >          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.

err is passed for two purposes:
* Debug messages;
* Calling error_free().

The function keeps doing both like before, but the difference is
that now it just takes ownership of err.

-- 
Eduardo