On 04/14/2016 04:29 AM, Alex Bligh wrote:
> Eric,
>
>> +Both options have identical formats for requests and replies. The only
>> +difference is that after a successful reply to `NBD_OPT_GO` (i.e. zero
>> +or more `NBD_REP_INFO` then an `NBD_REP_EXPORT`), transmission mode is
>> +entered immediately. Therefore these commands share common
>> +documentation.
>
> So just to check, for an *unsuccessful* option, can I send a sequence
> of NBD_REP_INFO followed by (e.g.) NBD_REP_ERR_TLSREQD?
Seems like a reasonable request, although I hadn't thought of it. It
risks leaking information, but there's nothing wrong with a SELECTIVETLS
server that doesn't mind leaking information.
> I think it
> would be useful if that was permitted, partly to convey information to
> the client about the mount in error conditions (of course it need
> not send any if it doesn't want to) but mostly because it would let
> the server incrementally find things out and send options, only
> errorring when it found the error, as opposed to buffering everything
> up and only releasing the buffer when it discovers there are no
> errors at all.
>
> If I'm right about this, it might be worth calling out explicitly.
Sure, I'll try to work that into my next draft.
>
>> +* `NBD_REP_INFO`
>> +
>> + A detailed description about an aspect of an export. The response
>> + to `NBD_OPT_INFO` and `NBD_OPT_GO` may include zero or more of
>> + these messages prior to the final `NBD_REP_EXPORT`
>
> If I'm right above 'the final `NBD_REP_EXPORT` or error'.
In fact, with your approach coupled with Wouter's desire to end on
NBD_REP_ACK rather than NBD_REP_EXPORT, it becomes 'zero or more
NBD_REP_INFO followed by NBD_REP_ERR_*, or at least one NBD_REP_INFO
followed by NBD_REP_ACK'
>
>> , although
>> + within the sequence of replies for a single export, the server
>> + MUST NOT send a given info type more than once.
>
> Perhaps 'unless that info type explicitly permits it'. I can see
> cases where an info type might want to list things. Given there
> are no such current cases and the client will ignore things it
> doesn't understand, I see no point encouraging clients to error this.
Okay. And thinking about it more, a server may indeed want to advertise
all names by which an export is known, so NBD_INFO_NAME could even be
given more than once (but then the wording needs to no longer call out
that it is a 'canonical' name).
>
>> Clients MUST
>> + ignore info types that it does not recognize.
>
> Para break?
>
>> The acceptable
>> + values for the header *length* field are determined by the info
>> + type, and includes the 2 bytes for the type designator, in the
>> + following general layout:
>> +
>> + - 16 bits, info type
>> + - *length - 2* bytes, info payload
>> +
>> + The following information types are defined:
>> +
>> + * `NBD_INFO_NAME` (1)
>> +
>> + Represents the server's canonical name of the export. The name
>> + MAY differ from the name presented in the client's option
>> + request,
>
> fine
>
>> and the info item SHOULD be omitted unless the server
>> + has multiple alternate names for a single export or a default
>> + export was specified.
>
> interesting - why 'SHOULD be omitted'? Why not leave it to the
> server? If it wants to send it because it's easier, then fair
> enough I think. If (e.g.) the server has case-insensitive export
> names, should it really determine whether the export name happens
> to be the same in upper case and lower case before sending this?
I guess there's nothing wrong with a server that wants to send it
always, even if there is no case-insensitivity or alternate naming in
use. But at the same time, I want to mention that it is not mandatory.
I'll come up with something.
>
>> The *length* MUST be at least 2, and the
>> + *info payload* is interpreted as:
>> +
>> + - String: name of the export, *length - 2* bytes
>
> In some other draft somewhere, you (I think) - else it was
> me - made the good point that we should keep string format consistent.
> So I'd rather this was
> - Length of string
> - String
The string format only needs a separate length IF you can further expand
the struct to return more than just the string. We already document for
NBD_OPT_INFO and NBD_OPT_SERVER_NAME that the string length is embedded
as the header length, without needing a second field for it. I see no
reason for the redundant string length here, since we won't be extending
individual info type payloads, but rather adding new info types (being
able to return more than one NBD_REP_INFO makes things a lot easier).
>
> Even though there is a duplicate length, as this theoretically
> allows the elements to be expanded (weak argument as we have other
> elements), is consistent with strings elsewhere (strong argument)
> and makes it easier for protocol analysers (strong argument).
Protocol analyzers already know the full length, by virtue of the common
NBD_REP_* header carrying that information. Next, they either recognize
the particular NBD_REP_INFO ("oh, read the first two bytes to see if I
further recognize the NBD_INFO_* for how to parse the payload") or they
don't ("NBD_REP_INFO is an unknown reply, skip to the next NBD_REP_* by
virtue of the overall length). But I don't see how a protocol analyzer
can ever assume that "oh, I don't recognize this particular NBD_INFO_*,
so let me parse the payload to see if there is a string length
embedded". The only way a protocol analyzer will treat the payload as a
string is if it knows the NBD_INFO_* type, at which point it also knows
all offsets within the type without needing redundant lengths.
Therefore, I don't think we need a second length within the payload of
NBD_INFO_NAME.
And I already argued above that not having a redundant length IS
consistent, at least with NBD_OPT_INFO and NBD_OPT_EXPORT_NAME.
NBD_REP_SERVER is a bit of an oddball, because we designed it to be
potentially extensible, but have (in the last couple weeks) decided that
extending it is not as clean as we first envisioned, and actually
requires more work on the client to ensure that the embedded length is
consistent with the overall length. On the other hand, I do have my
pending proposal to make structured reply NBD_REPLY_TYPE_ERROR* have an
embedded length. So maybe Wouter should make a decision on this topic.
>>
>>
>> +* `NBD_REP_EXPORT`
>> +
>> + The final reply to a successful `NBD_OPT_INFO` or `NBD_OPT_GO`,
>> + after zero or more `NBD_REP_INFO`. Describes the same information
>> + that is sent in response to the older `NBD_OPT_EXPORT_NAME`,
>> + except that there are no trailing zeroes whether or not
>> + `NBD_FLAG_C_NO_ZEROES` was negotiated. *length* MUST be 10, and
>> + the payload is interpreted as follows:
>> +
>> + - 64 bits, size of the export in bytes (unsigned)
>> + - 16 bits, transmission flags
>> +
>> ### `WRITE_ZEROES` extension
>
> OK.
Changes a bit to become another NBD_INFO_ constant if we want the series
to end in NBD_REP_ACK on success.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature