| Message ID | 20231231093016.14204-10-avihaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Misc cleanups and fixes | expand |
+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. > 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 <avihaih@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é <philmd@linaro.org>
Avihai Horon <avihaih@nvidia.com> writes: > 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. > > 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 <avihaih@nvidia.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
Philippe Mathieu-Daudé <philmd@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 <avihaih@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é <philmd@linaro.org>
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; }
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. 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 <avihaih@nvidia.com> --- migration/channel.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)