[Libguestfs] [PATCH v3 11/14] nbd/client: Accept 64-bit block status chunks

[Vladimir Sementsov-Ogievskiy]

On 31.05.23 20:40, Eric Blake wrote:
> On Wed, May 31, 2023 at 08:00:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 15.05.23 22:53, Eric Blake wrote:
>>> Because we use NBD_CMD_FLAG_REQ_ONE with NBD_CMD_BLOCK_STATUS, a
>>> client in narrow mode should not be able to provoke a server into
>>> sending a block status result larger than the client's 32-bit request.
>>> But in extended mode, a 64-bit status request must be able to handle a
>>> 64-bit status result, once a future patch enables the client
>>> requesting extended mode.  We can also tolerate a non-compliant server
>>> sending the new chunk even when it should not.
>>>
>>> @@ -672,7 +681,8 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
>>>         * connection; just ignore trailing extents, and clamp things to
>>>         * the length of our request.
>>>         */
>>> -    if (chunk->length > sizeof(context_id) + sizeof(*extent)) {
>>> +    if (count != wide ||
>>
>> hard to read for me. Could it be simply "count > 1 ||" ?
> 
> For existing commands (compact), count is initialized to 0 as it is
> not transferred over the wire.  For extended commands, count is
> transferred over the wire, but we expect it to be 1 (and not 0).
> Comparing count != wide is more precise than checking count > 0 (which
> should never happen for compact, but would be a bug for extended).

The only case you add to the check is when count = 0 for extended. But in this case "more than one extent" message is counterintuitive.

> 
>>
>>> +        chunk->length > sizeof(context_id) + wide * sizeof(count) + len) {
>>>            trace_nbd_parse_blockstatus_compliance("more than one extent");
>>>        }
>>>        if (extent->length > orig_length) {
>>> @@ -1117,7 +1127,7 @@ static int coroutine_fn nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t h
>>>
>>>    static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>>>                                                             uint64_t handle, uint64_t length,
>>> -                                                         NBDExtent *extent,
>>> +                                                         NBDExtentExt *extent,
>>>                                                             int *request_ret, Error **errp)
>>>    {
>>>        NBDReplyChunkIter iter;
>>> @@ -1125,6 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>>>        void *payload = NULL;
>>>        Error *local_err = NULL;
>>>        bool received = false;
>>> +    bool wide = false;
>>>
>>>        assert(!extent->length);
>>>        NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) {
>>> @@ -1134,7 +1145,13 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>>>            assert(nbd_reply_is_structured(&reply));
>>>
>>>            switch (chunk->type) {
>>> +        case NBD_REPLY_TYPE_BLOCK_STATUS_EXT:
>>> +            wide = true;
>>> +            /* fallthrough */
>>>            case NBD_REPLY_TYPE_BLOCK_STATUS:
>>> +            if (s->info.extended_headers != wide) {
>>> +                trace_nbd_extended_headers_compliance("block_status");
>>
>> You set wide to true once, on first "NBD_REPLY_TYPE_BLOCK_STATUS_EXT", and then parse following "NBD_REPLY_TYPE_BLOCK_STATUS" if the come with wide=true.
>>
>> Should it be:
>>
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -1135,7 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>>       void *payload = NULL;
>>       Error *local_err = NULL;
>>       bool received = false;
>> -    bool wide = false;
>> +    bool wide;
>>       assert(!extent->length);
>>       NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) {
>> @@ -1146,9 +1146,8 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>>           switch (chunk->type) {
>>           case NBD_REPLY_TYPE_BLOCK_STATUS_EXT:
>> -            wide = true;
>> -            /* fallthrough */
>>           case NBD_REPLY_TYPE_BLOCK_STATUS:
>> +            wide = chunk->type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT;
>>               if (s->info.extended_headers != wide) {
> 
> Good observation, since we reach this multiple times in a loop.  I'm
> squashing that in.
> 

-- 
Best regards,
Vladimir