Re: [Qemu-devel] [PATCH v2 5/7] block/nbd-client: nbd_co_send_request: return -EIO if s->quit was set in parallel

[Vladimir Sementsov-Ogievskiy]

18.09.2017 19:01, Eric Blake wrote:
> On 09/18/2017 08:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> > It's incorrect to return success rc >= 0 if we skip qio_channel_writev_all()
> > call due to s->quit.
> Does this need to cc: qemu-stable for 2.10.1 (or put another way, can we
> come up with some scenario of EAGAIN or other handling that would
> actually set s->quit in a parallel coroutine when a client sends out
> multiple requests at once)?

Intuitively, I think it should not be possible, first nbd_send_request 
should not do
something serious (should not yield) after set_cork..

> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   block/nbd-client.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/block/nbd-client.c b/block/nbd-client.c
> > index f331f08a9a..280147e6a7 100644
> > --- a/block/nbd-client.c
> > +++ b/block/nbd-client.c
> > @@ -189,6 +189,9 @@ static int nbd_co_send_request(BlockDriverState *bs,
> >       }
> >   
> >   err:
> > +    if (rc >= 0 && s->quit) {
> > +        rc = -EIO;
> > +    }
> I'm still not convinced this is in the right place.  This fails the
> send_request regardless of whether we skipped qio_channel_writev_all();
> shouldn't the rc be set only in the case that we actually skipped
> writing the full command because s->quit was detected at that point in time?
>
> >       if (rc < 0) {
> >           s->quit = true;
> >           s->requests[i].coroutine = NULL;

--
Best regards,
Vladimir