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

[Eric Blake]

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 <address@hidden>

> 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) {
> 

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

signature.asc
Description: OpenPGP digital signature