Re: [Qemu-devel] [Nbd] [PATCH] Strawman proposal for NBD structured replies

[Wouter Verhelst]

On Tue, Mar 29, 2016 at 10:59:18PM +0100, Alex Bligh wrote:
> On 29 Mar 2016, at 21:57, Wouter Verhelst <address@hidden> wrote:
> > 
> > I understand why you do it this way (we don't need 2^16 reply types),
> > but (in contrast to the flags in the request packet) this makes it
> > harder to specify flags and command type as separate fields (there is no
> > 24-bit integer on most systems).
> > 
> > As said though, I understand why, and the alternative isn't ideal.
> 
> As a third option then:
> 
> Each chunk consists of the following:
> 
> S: 32 bits, 0x668e33ef, magic (NBD_STRUCTURED_REPLY_MAGIC)
> S: 8 bits: type
> S: 8 bits: reserved (must be zero)
> S: 16 bits, flags
> S: 64 bits, handle
> S: 32 bits, payload length S: (length bytes of payload data)
> 
> The flags have the following meanings:
> 
> • bits 0-15: reserved (server MUST set these to zero)

That seems better in that context, yes. The reserved byte could later on
be assigned as extra flags if need be.

> >> +If the server detects an error during an operation which it
> >> +is serving with a structured reply, it MUST complete
> >> +the transmission of the current data chunk if transmission
> >> +has started (by padding the current chunk with data
> >> +which MUST be zero), after which zero or more other
> >> +data chunks may be sent, followed by an `NBD_CHUNKTYPE_END`
> >> +chunk. The server MAY set the offset within `NBD_CHUNKTYPE_END`
> >> +to the offset of the error; if so, this MUST be within the
> >> +length requested.
> > 
> > This should probably also be more explicit about what to do if the
> > server doesn't want to set the offset (set it to zero, presumably)
> 
> Hmm. Perhaps it would be better to set the offset to 2^32-1 to
> indicate "I don't know". Making this value useful is difficult in
> the situation where the server is running multiple sendfiles on
> multiple chunks.

(side note: you can't do multiple sendfile-like things concurrently; one
of them will require exclusive access to write to the socket)

> There could actually be multiple errors, and you
> don't want the server to rely on 'data up to X' as being OK as
> only one error is reported. I'd therefore suggest an error offset
> of 2^32-1 means 'one or more error, assume all delivered data is
> potentially erroneous'.

The reason why I suggested zero is that it doesn't require special-case
code. If an error offset implies that everything beyond that offset is
invalid, then having an offset of zero implies that the whole read is
invalid -- which is correct if the server encountered an error, but
doesn't know or doesn't want to say (for whatever reason) where.

Maybe the "MAY set the offset" above should just be a "MUST set the
offset", with the clarification that the offset "MUST not be beyond the
actual error location, but MAY be before it if the server has no
detailed information", or something along those lines.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12