Philippe Mathieu-Daudé <phi...@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 <avih...@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é <phi...@linaro.org>
