[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/5] vnc: No need for Error** parameter at vnc_c
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH 5/5] vnc: No need for Error** parameter at vnc_client_io_error() |
Date: |
Thu, 8 Jun 2017 14:44:51 -0300 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
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
- Re: [Qemu-devel] [PATCH 3/5] websock: Don't try to set *errp directly, (continued)
Re: [Qemu-devel] [PATCH 0/5] Error handling cleanup and fixes, Eric Blake, 2017/06/08