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 <ebl...@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabk...@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
signature.asc
Description: OpenPGP digital signature
