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

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

[Fiona Ebner]

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

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

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

[Juan Quintela]

Fiona Ebner <f.ebner@proxmox.com> wrote:
> 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>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Queued in migration tree.

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

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

[Fiona Ebner]

Am 17.10.22 um 11:54 schrieb 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?
> 

Thank you for the review! Unfortunately, I'll be pretty busy in the
coming weeks, because we have some releases coming up. But if nobody
else sends patches until those are done, I'll take a look then.

Best Regards,
Fiona

> Thanks
> Chen 
> 
>