On 03/31/2016 02:19 AM, Wouter Verhelst wrote: > We're getting close. > >> +[option #A1 - only replies with payload are affected] >> +[option #A2 - enabling structured replies MAY affect all other commands] >> +[option #A3 - enabling structured replies MUST affect all commands] > > I still think #1 is best, but I think Markus' input is valuable too, so > we should probably hold off on deciding this until we've seen his > opinion. > > I'm going to veto #3, though. Last night, when I posted the series, I was leaning towards #1. But after sleeping on it, and with Alex's comments, I'm now leaning towards #2. The idea of returning a structured error-with-offset on NBD_CMD_READ or NBD_CMD_TRIM (to let the client know that the first half of the request was honored, and only the second half is indeterminate) is an appealing argument FOR allowing (but not requiring) a structured reply. Another argument in favor of option #2: in v4, I plan to amend NBD_REPLY_TYPE_ERROR to explicitly allow the server to optionally send more than length 4 (and NBD_REPLY_TYPE_ERROR_OFFSET to send more than length 12) - anything beyond the fixed fields would then be a human-readable UTF-8 string (the same as we use for NBD_REP_ERR_* handshake replies). We can then document that any command may use a structured error-with-message on failures, but ONLY if structured replies have been negotiated, and merely make sure that for commands that do not REQUIRE a structured reply, that any structured reply that the command could usefully send will still map sanely back to the simple reply where there is nothing more than the error code. I am in agreement that option #3 is needlessly strong. > > Beyond that, LGTM. Alex had some good comments that will also affect a v4, and we're still waiting for Markus to get back and have a chance to chime in, so this patch is not quite ready for inclusion yet, but I'm liking the progress we've made. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature