Re: [PATCH 09/11] migration: Fix migration_channel_read_peek() error path

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> +Markus
>
> On 31/12/23 10:30, Avihai Horon wrote:
>> migration_channel_read_peek() calls qio_channel_readv_full() and handles
>> both cases of return value == 0 and return value < 0 the same way, by
>> calling error_setg() with errp. However, if return value < 0, errp is
>> already set, so calling error_setg() with errp will lead to an assert.
>
> I suppose the API would be safer by passing &len as argument and
> return a boolean.

Doubtful, unless I'm misunderstanding something.

Function comment:

 * Returns: the number of bytes read, or -1 on error,
 * or QIO_CHANNEL_ERR_BLOCK if no data is available
 * and the channel is non-blocking

I understand this as:

* Success case: return #bytes read

* Error case: return -1, @errp is set

* Would block case: return QIO_CHANNEL_ERR_BLOCK

A zero return value must be the success case.  I figure this can happen
only at EOF.  A caller might need to treat unexpected EOF as an error.

>> Fix it by handling these cases separately, calling error_setg() with
>> errp only in return value == 0 case.
>> Fixes: 6720c2b32725 ("migration: check magic value for deciding the mapping of channels")
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> ---
>>   migration/channel.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>> diff --git a/migration/channel.c b/migration/channel.c
>> index ca3319a309..f9de064f3b 100644
>> --- a/migration/channel.c
>> +++ b/migration/channel.c
>> @@ -117,9 +117,12 @@ int migration_channel_read_peek(QIOChannel *ioc,
>>          len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL,
>>                                       QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
>> -        if (len <= 0 && len != QIO_CHANNEL_ERR_BLOCK) {
>> -            error_setg(errp,
>> -                       "Failed to peek at channel");
>> +        if (len < 0 && len != QIO_CHANNEL_ERR_BLOCK) {
>> +            return -1;
>> +        }
>> +
>> +        if (len == 0) {
>> +            error_setg(errp, "Failed to peek at channel");
>>              return -1;
>>           }
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>