On 04/01/2016 04:33 AM, Alex Bligh wrote: > Eric, > > I know this has now been merged, but I have a few comments still (sorry). No problems. I'll post a patch incorporating most of your tweaks shortly (well, some of them I reworded slightly differently). >> + A structured reply in the transmission phase consists of one or >> + more structured reply chunk messages. The server MUST NOT send >> + this reply type unless the client has successfully negotiated >> + structured replies via `NBD_OPT_STRUCTURED_REPLY`. Conversely, if >> + structured replies are negotiated, the server MUST use a >> + structured reply for any response with a payload, and MUST NOT use >> + a simple reply for `NBD_CMD_READ` (even for the case of an early >> + `EINVAL` due to bad flags). > > I think we are left with an ambiguity. If > structured replies HAVE been negotiated, this doesn't say what > the server MUST/MAY do with replies that do *not* have a payload. > Also it's not clear whether a read response with an error immediate > error 'has a payload'. > > I don't want to rehash the discussion but please could the text > definitively set the position out (one way or another). I'm definitively stating that for all other replies, the server MAY use either simple or structured reply, but that when reporting an error, a server SHOULD use structured reply to take advantage of a payload of a human-readable message. >> + * Structured Reply types >> + >> + These values are used in the "type" field of a structured reply. >> + Some chunk types can additionally be categorized by role, such as >> + *error chunks* or *content chunks*. Each type determines how to >> + interpret the "length" bytes of payload. If the client receives >> + an unknown or unexpected type, it SHOULD close the connection. > > Nit: this is a 'MUST' everywhere else. I would have thought either > 'MUST ignore', or 'MUST close' > MUST ignore may be a bit strong (if the server sends a new class of error message that the client doesn't know about, ignoring the unknown class will make the client treat the command as success while the server was trying to convey an error). Of course, the server shouldn't be sending new reply types that the client didn't negotiate, but I'm leaning towards MUST close. >> + SHOULD NOT mix errors with offsets with a generic error. As long >> + as all errors are accompanied by offsets, the client MAY assume >> + that any data chunks with no subsequent error offset are valid, >> + that chunks with an overlapping error offset errors are valid up >> + until the reported offset, and that portions of the read that do >> + not have a corresponding content chunk are not valid. >> + >> + A client MAY close the connection if it detects that the server > > MUST? > >> + has sent invalid chunks (such as overlapping data, or not enough >> + data before claiming success). I'm intentionally using MAY here - depending on the client implementation, it is an extra burden to validate that the server didn't reply with junk. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature