On 19/07/2019 17:44, Eric Blake wrote: > On 7/19/19 9:34 AM, Eric Blake wrote: >> On 7/19/19 4:40 AM, Andrey Shinkevich wrote: >>> In case nbd_co_receive_one_chunk() fails in >>> nbd_reply_chunk_iter_receive(), 'NBDReply reply' parameter is used in >>> the check nbd_reply_is_simple() without being initialized. The iotest >>> 083 does not pass under the Valgrind: $./check -nbd -valgrind 083. >>> The alternative solution is to swap the operands in the condition: >>> 'if (s->quit || nbd_reply_is_simple(reply))' >>> >>> Signed-off-by: Andrey Shinkevich <andrey.shinkev...@virtuozzo.com> >>> --- >>> block/nbd.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Huh. Very similar to >> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html, but >> affects a different function. I can queue this one through my NBD tree >> to get both in my rc2 pull request. >> >> Reviewed-by: Eric Blake <ebl...@redhat.com> > > Actually, since this is the second patch on the same topic, I'm > wondering if it's better to use the following one-liner to fix BOTH > issues and without relying on a gcc extension: > > diff --git i/block/nbd.c w/block/nbd.c > index 8d565cc624ec..f751a8e633e5 100644 > --- i/block/nbd.c > +++ w/block/nbd.c > @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( > request_ret, qiov, payload, > errp); > > if (ret < 0) { > + memset(reply, 0, sizeof *reply);
The call to memset() consumes more CPU time. I don't know how frequent the "ret < 0" path is. The initialization ' = {0}' is cheaper. Is it safe to swap the operands in the condition in nbd_reply_chunk_iter_receive(): 'if (s->quit || nbd_reply_is_simple(reply))' ? Andrey > s->quit = true; > } else { > /* For assert at loop start in nbd_connection_entry */ > -- With the best regards, Andrey Shinkevich