migration/channel-block.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
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
> -----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
>
>
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
>
>
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.
© 2016 - 2024 Red Hat, Inc.