Re: [Qemu-devel] [PATCH v2 1/6] nbd/server: nbd_negotiate: return 1 on NBD_OPT_ABORT

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]

On 06/29/2017 01:46 PM, Eric Blake wrote:
> On 06/21/2017 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Separate case when client sent NBD_OPT_ABORT from other errors.
> 
> Commit messages are best written in imperative tense (you can supply an
> implicit "apply this patch in order to" prefix in front of the message,
> and it should still generally read well); and that doesn't mix well with
> past-tense descriptions.  I might have written:
> 
> Separate the case when a client sends NBD_OPT_ABORT from all other errors.
> 
>> It will be needed for the following patch, where errors will be
>> reported.
>> Considered case is not actually the error - it honestly follows NBD
>> protocol. Therefore it should not be reported like an error.
> 
> This particular case is not actually an error - it honestly follows the
> NBD protocol.
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  nbd/server.c | 20 +++++++++++++++-----
>>  1 file changed, 15 insertions(+), 5 deletions(-)
>>
> 
>> -
>> -/* Process all NBD_OPT_* client option commands.
>> - * Return -errno on error, 0 on success. */
>> +/* nbd_negotiate_options
>> + * Process all NBD_OPT_* client option commands.
>> + * Return:
>> + * -errno  on error
>> + * 0       on successful negotiation
>> + * 1       if client sent NBD_OPT_ABORT, i.e. on legal disconnect
> 

Phooey, I'm just now noticing (while trying to rebase my existing
patches on top of yours) that we have a conflict in semantics.  For
reference, my patch was posted here:

https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04535.html

where I picked the semantics:

+/* Process all NBD_OPT_* client option commands, during fixed newstyle
+ * negotiation. Return -errno on error, 0 on successful
NBD_OPT_EXPORT_NAME,
+ * and 1 on successful NBD_OPT_GO. */
+static int nbd_negotiate_options(NBDClient *client, uint16_t myflags)

But since I'm rebasing my stuff anyways, I'll come up with some other
way to combine the two semantics (perhaps by refactoring the response to
NBD_OPT_EXPORT_NAME to occur in nbd_negotiate_options() instead of in
the caller, so that returning 0 is always sufficient to mark that
newstyle negotiation is complete and the server is now in transmission
phase).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]