Re: [Qemu-block] [PATCH 6/9] nbd: BLOCK_STATUS for standard get_block_status function: client part

[Vladimir Sementsov-Ogievskiy]

16.02.2018 23:40, Eric Blake wrote:
> On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> > Minimal realization: only one extent in server answer is supported.
> > Flag NBD_CMD_FLAG_REQ_ONE is used to force this behavior.

[...]

> > +    memcpy(extent, payload, sizeof(*extent));
> > +    be32_to_cpus(&extent->length);
> > +    be32_to_cpus(&extent->flags);
>
> Instead of doing a memcpy() and then in-place bit-swizzling, you could 
> do the swapping as part of assignment, for one less function call (and 
> make the code a bit easier to extend, if we later drop our REQ_ONE 
> limitation on only having one extent, because you'll advance payload 
> as needed):
>
> extent->length = payload_advance32(&payload);
> extent->flags = payload_advance32(&payload);

Aha, yes. The funny thing is that these are my own helpers.

> We should probably validate that the length field is a multiple of 
> min_block (if a server tells us that all operations must be 512-byte 
> aligned, then reports an extent that is smaller than 512 bytes, we 
> have no way to ask for the status of the second half of the sector). 
> Probably also something that needs to be explicitly stated in the NBD 
> spec. [1]
>
> > +
> > +    if (extent->length > orig_length) {
> > +        error_setg(errp, "Protocol error: server sent chunk 
> > exceeding requested"
> > +                         " region");
> > +        return -EINVAL;
>
> That matches the current spec wording, but I'm not sure I agree with 
> it - what's wrong with a server providing a final extent that extends 
> beyond the request, if the information was already available for free 
> (the classic example: if the server never replies with HOLE or ZERO, 
> then the entire file has the same status, so all requests could 
> trivially be replied to by taking the starting offset to the end of 
> the file as the returned length, rather than just clamping at the 
> requested length).

Maybe. But our already released clients not prepared to such change =(

But, on the other hand, this gives us possibility to understand, the the 
whole target (for backup/mirror) is zero in one request, skipping 
target-zeroing loop by 4gb chunks.. What about adding such possibility 
with an additionally negotiated option or something like this? (and 
don't we now have same possibility with something like INFO?)

> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   /* nbd_parse_error_payload
> >    * on success @errp contains message describing nbd error reply
> >    */

--
Best regards,
Vladimir