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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

02.06.2017 01:33, Eric Blake wrote:
> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Separate case when client sent NBD_OPT_ABORT from 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.
>> -EPIPE case means client not read server reply on NBD_OPT_ABORT,
>> which is also OK.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/server.c | 20 +++++++++++++++-----
>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 30dfb81a5c..0e53d3dd91 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -369,9 +369,13 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
>>       return QIO_CHANNEL(tioc);
>>   }
>>   
>> -
>> -/* 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:
>> + * < 0 on error
> Do you want to be specific that this is a negative errno value, or is it
> just any negative value with no correlation to errno?

nothing here (except blk_write and friends, but their errors are not 
returned by any function) correlates with errno, because core function - 
nbd_wr_syncv returns EIO on any error. Underlying io_channel returns -1 
or QIO_CHANNEL_ERR_BLOCK...


>
>> + * 0   on successful negotiation
>> + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
>> + */
>>   static int nbd_negotiate_options(NBDClient *client)
>>   {
>>       int ret;
>> @@ -483,7 +487,7 @@ static int nbd_negotiate_options(NBDClient *client)
>>                   }
>>                   /* Let the client keep trying, unless they asked to quit */
>>                   if (clientflags == NBD_OPT_ABORT) {
>> -                    return -EINVAL;
>> +                    return 1;
>>                   }
>>                   break;
>>               }
>> @@ -502,7 +506,7 @@ static int nbd_negotiate_options(NBDClient *client)
>>                    * guests that don't wait for our reply. */
>>                   ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
>>                                                clientflags);
>> -                return ret < 0 ? ret : -EINVAL;
>> +                return ret < 0 && ret != -EPIPE ? ret : 1;
> This should just be 'return 1;', which means you don't need to capture
> and check 'ret'.
>
>>   
>>               case NBD_OPT_EXPORT_NAME:
>>                   return nbd_negotiate_handle_export_name(client, length);
>> @@ -560,6 +564,12 @@ static int nbd_negotiate_options(NBDClient *client)
>>       }
>>   }
>>   
>> +/* nbd_negotiate
>> + * Return:
>> + * < 0 on error
> Again, if this is reliably a negative errno, specifically document that.
>
>> + * 0   on successful negotiation
>> + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
>> + */
>>   static coroutine_fn int nbd_negotiate(NBDClient *client)
>>   {
>>       char buf[8 + 8 + 8 + 128];
>>

-- 
Best regards,
Vladimir