RE: [PATCH v2] migration/channel-block: fix return value for qio_channel_block_{readv, writev}

[Zhang, Chen]

> -----Original Message-----
> From: Qemu-devel <qemu-devel-bounces+chen.zhang=intel.com@nongnu.org>
> On Behalf Of Fiona Ebner
> Sent: Thursday, October 13, 2022 4:41 PM
> To: qemu-devel@nongnu.org
> Cc: dgilbert@redhat.com; quintela@redhat.com; berrange@redhat.com
> Subject: [PATCH v2] migration/channel-block: fix return value for
> qio_channel_block_{readv, writev}
> 
> in the error case. The documentation in include/io/channel.h states that -1 or
> QIO_CHANNEL_ERR_BLOCK should be returned upon error. Simply passing
> along the return value from the bdrv-functions has the potential to confuse
> the call sides. Non-blocking mode is not implemented currently, so -1 it is.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

LGTM.
Reviewed-by: Zhang Chen <chen.zhang@intel.com>

But I found the same problem elsewhere, for example: 
"qio_channel_rdma_writev/readv", "qio_channel_buffer_writev/readv"...etc...

Can you send other patches to fix it?

Thanks
Chen 


> ---
> 
> v1 -> v2:
>     * Use error_setg_errno() instead of error_setg().
>     * Use "failed" instead of "returned error" in error message. Now
>       that no numerical error code is used, this sounds a bit nicer
>       IMHO.
> 
>  migration/channel-block.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/channel-block.c b/migration/channel-block.c index
> c55c8c93ce..f4ab53acdb 100644
> --- a/migration/channel-block.c
> +++ b/migration/channel-block.c
> @@ -62,7 +62,8 @@ qio_channel_block_readv(QIOChannel *ioc,
>      qemu_iovec_init_external(&qiov, (struct iovec *)iov, niov);
>      ret = bdrv_readv_vmstate(bioc->bs, &qiov, bioc->offset);
>      if (ret < 0) {
> -        return ret;
> +        error_setg_errno(errp, -ret, "bdrv_readv_vmstate failed");
> +        return -1;
>      }
> 
>      bioc->offset += qiov.size;
> @@ -86,7 +87,8 @@ qio_channel_block_writev(QIOChannel *ioc,
>      qemu_iovec_init_external(&qiov, (struct iovec *)iov, niov);
>      ret = bdrv_writev_vmstate(bioc->bs, &qiov, bioc->offset);
>      if (ret < 0) {
> -        return ret;
> +        error_setg_errno(errp, -ret, "bdrv_writev_vmstate failed");
> +        return -1;
>      }
> 
>      bioc->offset += qiov.size;
> --
> 2.30.2
> 
>