On 04/20/2016 04:33 AM, Alex Bligh wrote: > Eric, > > Thanks for breaking it up. > > > By embedding string lengths inside the data > you've made the existing types inherently expandable, which is good. Intentionally only the error types, and there, the length is ONLY expandable by the error message, not by anything else. Rather than expanding existing types, we should create new types any time we want to return additional information. > But you need to make this clearer. I would add "The client MUST skip any data > within the *length* field of the reply header which is unused > by the specified reply types below; this area is reserved for > expansion." (or similar) Not for regular chunked replies, and for error replies, I did that. But I can tweak the wording, if it helps. >> + >> +- `NBD_REPLY_TYPE_ERROR` (2^15 + 1) >> + >> + This chunk type is in the error chunk category. *length* MUST >> + be at least 6. This chunk represents that an error occurred, >> + and the client MAY NOT make any assumptions about partial > > that should be 'MUST NOT' (existing problem). Okay. > >> + success. This type SHOULD NOT be used more than once in a >> + structured reply. Valid as a reply to any request. >> + >> + The payload is structured as: >> + >> + 32 bits: error (MUST be nonzero) >> + 16 bits: message length (no more than header *length* - 6) >> + *message length* bytes: optional string suitable for >> + direct display to a human being > > Everywhere else where we have a string we have a 32 byte message > length. For the sake of consistency, given this is hardly > speed criticial, perhaps we could stick with that. But strings MUST NOT exceed 4096 bytes, which means 32 bits is wasting 2 bytes over the wire. I see no reason to make it easier for a rogue server to do a denial of service attack on a guest by making it possible to pass a length larger than 64k when 4096 is already the upper limit a client is expecting. >> + 32 bits: error (MUST be non-zero) >> + 16 bits: message length (no more than header *length* - 14) >> + *message length* bytes: optional string suitable for >> + direct display to a human being > > Same point re 16/32 bit string lengths > >> + 64 bits: offset (unsigned) > > Perhaps put the offset before the message? I'm just thinking > about the implementor who would probably read things into > a struct if they have the chance. No. The implementor KNOWS that a message is at offset 6 within the message, and that the string length is at offset 4, without knowing the interpretation of the rest of the payload. So the rest of the payload MUST come after the message, in order for this scheme of the message always being at a known location to work. > > That's how it was before, and you seem to have reordered (not > sure why). Because of our earlier conversation on the same topic: https://sourceforge.net/p/nbd/mailman/nbd-general/thread/20160409104438.GP19023%40grep.be/#msg35003527 -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature