| Message ID | 20221013084100.57740-1-f.ebner@proxmox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] migration/channel-block: fix return value for qio_channel_block_{readv, writev} | expand |
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.
> -----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 > >
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;
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(-)