Re: [Qemu-block] [PATCH for-2.11] nbd: Don't crash when server reports NBD_CMD_READ failure

[Vladimir Sementsov-Ogievskiy]

12.11.2017 04:39, Eric Blake wrote:
> If a server fails a read, for example with EIO, but the connection
> is still live, then we would crash trying to print a non-existent
> error message.  Bug introduced in commit f140e300.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   block/nbd-client.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index b44d4d4a01..5f3375a970 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -78,7 +78,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
>       while (!s->quit) {
>           assert(s->reply.handle == 0);
>           ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);

hmm.

/* nbd_receive_reply
 * Returns 1 on success
 *         0 on eof, when no data was read (errp is not set)
 *         negative errno on failure (errp is set)
 */
int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)


- looks like errp should be set if ret < 0. may be the function should 
be fixed?
- don't you think that this function returns transferred server error? 
it doesn't.

> -        if (ret < 0) {
> +        if (local_err) {
>               error_report_err(local_err);
>           }
>           if (ret <= 0) {
> @@ -681,7 +681,7 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t 
> offset,
>
>       ret = nbd_co_receive_cmdread_reply(client, request.handle, offset, qiov,
>                                          &local_err);
> -    if (ret < 0) {
> +    if (local_err) {
>           error_report_err(local_err);
>       }
>       return ret;

but here, it looks like you are right. My first attempt was to store 
server error to ret and
server error msg to errp (bad idea, as I can see now), but you proposed 
to do not print every server error msg and
deleted this functionality. And now we have a set of functions which can 
return ret < 0
and not set errp.. It looks very confusing, I think. Better solution is 
to refactor this somehow,
may be not mixing server-error-reply with local normal errors..

For now, for second chunk:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>



--
Best regards,
Vladimir