Re: [Qemu-block] [PATCH v2 6/7] block/nbd-client: early fail nbd_read_reply_entry if s->quit is set

[Eric Blake]

On 09/18/2017 08:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> Do not continue any operation if s->quit is set in parallel.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/nbd-client.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 280147e6a7..f80a4c5564 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -81,7 +81,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
>          if (ret < 0) {
>              error_report_err(local_err);
>          }
> -        if (ret <= 0) {
> +        if (ret <= 0 || s->quit) {
>              break;
>          }

I'm still not convinced this helps: either nbd_receive_reply() already
returned an error, or we got a successful reply header, at which point
either the command is done (no need to abort the command early - it
succeeded) or it is a read command (and we should detect at the point
where we try to populate the qiov that we don't want to read any more).
It depends on your 3/7 patch for the fact that we even try to read the
qiov directly in this while loop rather than in the coroutine handler,
where Paolo questioned whether we need that change.

> @@ -105,9 +105,8 @@ static coroutine_fn void nbd_read_reply_entry(void 
> *opaque)
>              assert(qiov != NULL);
>              assert(s->requests[i].request->len ==
>                     iov_size(qiov->iov, qiov->niov));
> -            if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
> -                                      NULL) < 0)
> -            {
> +            ret = qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov, NULL);
> +            if (ret < 0 || s->quit) {
>                  s->requests[i].ret = -EIO;
>                  break;
>              }

The placement here looks odd. Either we should not attempt the read
because s->quit was already set (okay, your first addition makes sense
in that light), or we succeeded at the read (at which point, why do we
need to claim it failed?).

I'm trying to look forward to structured reads, where we will have to
deal with more than one server message in reply to a single client
request.  For read, we just piece together portions of the qiov until
the server has sent us all the pieces.  But for block query, I really do
think we'll want to defer to specialized handlers for doing further
reads (the amount of data to be read from the server is not known by the
client a priori, so it is a two-part read, one to get the length, and
another to read that remaining length); if we need some sort of callback
function rather than cramming all the logic here in the main read loop,
then the qio_channel_readv_all for read commands would belong in the
callback, and it is more a question of the main read loop checking
whether there is an early quit condition before calling into the callback.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature