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

[Eric Blake]

On 03/30/2016 01:33 AM, Wouter Verhelst wrote:
> Morning,
> 
> On Wed, Mar 30, 2016 at 07:59:15AM +0100, Alex Bligh wrote:
>> On 30 Mar 2016, at 00:17, Eric Blake <address@hidden> wrote:
>>>>
>>>> -The server replies with:
>>>> +Replies take one of two forms. They may either be structured replies,
>>>
>>> Hmm, you put your strawman directly in the 'transmission phase' section,
>>> while mine is deferred to the 'Experimental Extensions' section, at
>>> least until we have a reference client and server implementation.
>>
>> Yeah, my wording was straw man but I think it should go into the main
>> body of the work. Obviously in that case it wouldn't be *merged* until
>> we had a working implementation.
>>
>> The SELECT stuff is a bit different as I am not sure it was intended
>> to be standardized short term (i.e. it was a longer term experiment
>> IIRC).
> 
> Actually, I plan to implement that when I get around to doing STARTTLS
> (which I've started work on, but is far from ready).

On that tangent, I found SELECT slightly ambiguous (particularly since
I'm also considering a proposal to modify NBD_REP_SERVER to expose
alignment details, so it would have to play nicely with SELECT):

Based on normative text alone, we would have:

S: 64 bits, 0x3e889045565a9
S: 32 bits, NBD_OPT_SELECT (6)
S: 32 bits, NBD_REP_SERVER (2)
S: 32 bits, length
S: 32 bits, name length
S: 'name length' bytes, UTF-8 encoded name (is it NUL-terminated?)
S: 'length - name length' bytes, which is UTF-8 encoded message to
display to human

This implies that 'name length' <= 'length - 4', although we don't
actually state that the server MUST NOT send a name length longer than
that.  It might not hurt to also require that length include space for a
NUL terminator (in both the name, and in the optional human-readable
information field), but only if that matches existing practice (if it
does not, then we should document that the client and server are NOT
dealing with NUL-terminated UTF-8 strings, but relying on length instead).

The SELECT text then refines this:

S: 64 bits, 0x3e889045565a9
S: 32 bits, NBD_OPT_SELECT (6)
S: 32 bits, NBD_REP_SERVER (2)
S: 32 bits, length
S: 32 bits, name length
S: 'name length' bytes, UTF-8 encoded name (is it NUL-terminated?)
S: 64 bits, size
S: 16 bits, export flags

but doesn't mention what happens if 'length' > 'name length + 14'.
Presumably, if the server wants to include the same UTF-8 human
information as before, it would go AFTER the 16 bits for the export
field (in other words, the SELECT extension is carving out fields in the
MIDDLE of the payload).

So, once I know for sure about the handling of NUL bytes, I'll probably
try my hand at a patch to clarify the wording in both the normative and
in the SELECT extension.

>> I guess Wouter should be the arbiter of whether he'd prefer to merge
>> it only when we have an implementation. My concern is that it may
>> hang around in 'experimental', when it needs properly merging, which
>> will require re-wordsmithing.
> 
> I'll merge whatever the outcome of this discussion is in the
> experimental section; that way, it won't get lost. I can reformulate
> your text to fit there myself if needs be, although obviously having
> something that doesn't require such work is preferable. However, I'm
> leaning more towards Eric's proposal at this time, because it feels more
> mature.

Okay, I'll do my best to word things in the experimental section so that
we can minimize edits when moving text. Maybe I'll even go so far as to
post a 2-patch series; one with the text in the experimental section for
committing now; the other for moving the text (but NOT to be committed)
to the normative section, for demonstrating the level of effort required.


> 
> I now realize, after having slept over it, that you guys probably meant
> for an error-with-offset to only mark bytes that were part of *that*
> particular reply chunk to be marked as faulty, which makes more sense
> than "the whole request range from that point on", as I was interpreting
> it...

Indeed, anything I can do to make the wording more clear is welcome.
Obviously, a server can mark an entire data chunk invalid by setting the
offset to the same value as the offset used in that chunk; the real
power is that an offset that is > chunk-offset but < 'chunk-offset +
chunk-length' then lets the client know that the first half of chunk was
valid, the second half of chunk is bogus, and no other chunk is
impacted.  And when a lazy server sends error-without-offset, the client
is forced to treat ALL chunks as invalid; which is why I state that the
server SHOULD NOT mix error-with-offset and error-without-offset in the
same reply (SHOULD NOT, but not MUST NOT, because I can't predict every
possible error scenario, and there may be some fatal chain of events
where the server is forced to mix after all).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature