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

[Wouter Verhelst]

On Wed, Mar 30, 2016 at 12:43:57PM -0600, Eric Blake wrote:
> 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

Formally, it defines that as "the rest of the data contains some
undefined implementation-specific details about the export (...) [i]f
the client did not explicitly request otherwise, these details are
defined to be UTF-8 encoded data suitable for direct display to a human
being."

> 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'.

The idea here was that the client in case of the SELECT extension *does*
"explicitly request otherwise", meaning, the details about the export
here are no longer undefined or implementation-specific, but instead
clearly defined to be the "size" plus the "export flags".

> 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).

Sortof. I can see how you come to that conclusion, but it wasn't meant
as such. Then again, I'll readily admit that I didn't spend quite as
much thinking/discussing about the SELECT extension as compared to the
discussion we're having now :-)

> 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.

nbd-server.c does this:

        for(i=0; i<servers->len; i++) {
                SERVER* serve = &(g_array_index(servers, SERVER, i));
                len = htonl(strlen(serve->servename));
                memcpy(buf, &len, sizeof(len));
                strcpy(ptr, serve->servename);
                send_reply(opt, net, NBD_REP_SERVER, 
strlen(serve->servename)+sizeof(len), buf);
        }

where ptr = buf + sizeof(len).

(on a sidenote, that should really be a strncpy. Let me fix that...
there, done)

so it doesn't send the NUL byte. The nbd-client side ends up adding a
NUL byte after whatever the server sent; I think that makes the most
sense, since a client should do so anyway for data it receives from a
(potentially evil) peer.

[...]
> > 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.

You should probably be explicit about which parts of the response are
marked as invalid when an error is sent.

Also, what happens if a server which checks read()'s return value before
forming a header encounters an error? It wouldn't send the broken data
to the client (it knows it's broken), it wouldn't send any padding, so
it would send an error code to the client about an offset that it will
never send?

> 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.

Right. That seems like it's close to a clear spec :-)

> 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).

"someone just ate my filesystem," for instance :-)

-- 
< 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