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 01:05:16PM -0300, Eduardo Habkost wrote:
> 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?

What about the following?

Signed-off-by: Eduardo Habkost <address@hidden>
---
 ui/vnc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ui/vnc.c b/ui/vnc.c
index 51f13f0c29..cb55554210 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1180,6 +1180,15 @@ void vnc_disconnect_finish(VncState *vs)
     g_free(vs);
 }
 
+
+/*
+ * Handle I/O error (@ret < 0) or EOF (@ret == 0) from I/O
+ * channel.  In case of errors or EOF, @err is freed using
+ * error_free().
+ *
+ * Returns 0 in case @ret <= 0 and the error was properly
+ * handled, otherwise returns @ret.
+ */
 ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
 {
     if (ret <= 0) {
-- 
2.11.0.259.g40922b1

-- 
Eduardo