Re: [Qemu-devel] [PATCH for-4.0] nbd: Permit simple error to NBD_CMD_BLOCK_STATUS

[Eric Blake]

On 3/25/19 5:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 23.03.2019 17:24, Eric Blake wrote:
>> The NBD spec is clear that when structured replies are active, a
>> simple error reply is acceptable to any command except for
>> NBD_CMD_READ.  However, we were mistakenly requiring structured errors
>> for NBD_CMD_BLOCK_STATUS, and hanging up on a server that gave a
>> simple error (since qemu does not behave as such a server, we didn't
>> notice the problem until now).  Broken since its introduction in
>> commit 78a33ab5 (v2.12).
>>
>> Howeve, even if we had gotten it correct to accept simple errors back
>> then, we run into another problem: the client treats the server's
>> reply as fatal and hangs up on the connection, instead of merely
>> failing the block status request but being ready for the next
>> command. Broken in commit 7f86068d (unreleased).
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>

>> +++ b/block/nbd-client.c
>> @@ -718,9 +718,7 @@ static int 
>> nbd_co_receive_blockstatus_reply(NBDClientSession *s,
>>       bool received = false;
>>
>>       assert(!extent->length);
>> -    NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
>> -                            NULL, &reply, &payload)
>> -    {
>> +    NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) 
>> {

This part changes things to permit a simple reply if that reply is an
error. In turn, if the server sends a simple reply error,
iter.request_ret is set to the error returned by the server while
leaving iter.ret and iter.err unset (because we successfully got the
server's reply and can keep the connection alive), with commit 7f86068d
in play (prior to that commit, iter.ret was left 0 because the server
replied successfully, while iter.err was set to the set to the server's
error, while leaving iter.fatal unset).

>>           int ret;
>>           NBDStructuredReplyChunk *chunk = &reply.structured;
>>
>> @@ -758,9 +756,11 @@ static int 
>> nbd_co_receive_blockstatus_reply(NBDClientSession *s,
>>           payload = NULL;
>>       }
>>
>> -    if (!extent->length && !iter.err) {
>> -        error_setg(&iter.err,
>> -                   "Server did not reply with any status extents");
>> +    if (!extent->length && !iter.request_ret) {
> 
> Hmm, I don't see, what is changed. You just don't set ret and err if there 
> already
> set request_ret.. So, finally it may lead to other return code in 
> nbd_client_co_block_status
> and local_err will not be set and traced. But there is no connection close in 
> this case
> in current code, s->quit is not set.

And that's okay. The whole point of this is that if the server replied
with an error message (iter.request_ret), then it is okay that the
server did not give us any extent->length, and the caller will turn
iter.request_ret into the EINVAL reply (or whatever other errno value
the server sent) to .bdrv_co_block_status() while keeping the connection
alive.  But if the server did NOT send an error reply (iter.request_ret
is 0), but also did not send us any status, then we need to turn that
into an error condition (because our caller expected us to make progress
on success, even though the server did not make progress).

> 
>> +        if (!iter.err) {
>> +            error_setg(&iter.err,
>> +                       "Server did not reply with any status extents");
>> +        }
>>           if (!iter.ret) {
>>               iter.ret = -EIO;
>>           }
>>
> 
> 

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

signature.asc
Description: OpenPGP digital signature