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(-)
diff --git a/ui/vnc.h b/ui/vnc.h
index 694cf32ca9..52f65bbd86 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -526,7 +526,7 @@ uint32_t read_u32(uint8_t *data, size_t offset);
/* Protocol stage functions */
void vnc_client_error(VncState *vs);
-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);
void start_client_init(VncState *vs);
void start_auth_vnc(VncState *vs);
diff --git a/ui/vnc.c b/ui/vnc.c
index 47b49c7318..51f13f0c29 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1180,7 +1180,7 @@ void vnc_disconnect_finish(VncState *vs)
g_free(vs);
}
-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)
{
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);
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);
}
@@ -1344,7 +1341,7 @@ ssize_t vnc_client_read_buf(VncState *vs, uint8_t *data, size_t datalen)
ret = qio_channel_read(
vs->ioc, (char *)data, datalen, &err);
VNC_DEBUG("Read wire %p %zd -> %ld\n", data, datalen, ret);
- return vnc_client_io_error(vs, ret, &err);
+ return vnc_client_io_error(vs, ret, err);
}
--
2.11.0.259.g40922b1
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
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 <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.
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
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 <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.
>
> 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 <ehabkost@redhat.com>
---
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
On 06/08/2017 12:44 PM, Eduardo Habkost wrote:
>>>> -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.
Yes, and when you frame it that way, it makes sense. But with no comment
framing it that way, ...
>> 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?
... why yes, you've hit on why I felt uneasy - we are missing good
documentation!
>
> What about the following?
>
Yes, that makes it MUCH easier to understand what's going on.
With this squashed in,
Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> 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) {
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
© 2016 - 2025 Red Hat, Inc.