On 18/09/2023 22:41, Markus Armbruster wrote: > QIOChannelClass methods qio_channel_rdma_readv() and > qio_channel_rdma_writev() violate their method contract when > rdma->error_state is non-zero: > > 1. They return whatever is in rdma->error_state then. Only -1 will be > fine. -2 will be misinterpreted as "would block". Anything less > than -2 isn't defined in the contract. A positive value would be > misinterpreted as success, but I believe that's not actually > possible. > > 2. They neglect to set an error then. If something up the call stack > dereferences the error when failure is returned, it will crash. If > it ignores the return value and checks the error instead, it will > miss the error. > > Crap like this happens when return statements hide in macros, > especially when their uses are far away from the definition. > > I elected not to investigate how callers are impacted. > > Expand the two bad macro uses, so we can set an error and return -1. > The next commit will then get rid of the macro altogether. > > Signed-off-by: Markus Armbruster <arm...@redhat.com>
Reviewed-by: Li Zhijian <lizhij...@fujitsu.com> > --- > migration/rdma.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/migration/rdma.c b/migration/rdma.c > index 2173cb076f..30e6dff875 100644 > --- a/migration/rdma.c > +++ b/migration/rdma.c > @@ -2761,7 +2761,11 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc, > return -1; > } > > - CHECK_ERROR_STATE(); > + if (rdma->error_state) { > + error_setg(errp, > + "RDMA is in an error state waiting migration to abort!"); > + return -1; > + } > > /* > * Push out any writes that > @@ -2847,7 +2851,11 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc, > return -1; > } > > - CHECK_ERROR_STATE(); > + if (rdma->error_state) { > + error_setg(errp, > + "RDMA is in an error state waiting migration to abort!"); > + return -1; > + } > > for (i = 0; i < niov; i++) { > size_t want = iov[i].iov_len;