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
+ * 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;
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
+ * 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];
--
2.11.1
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?
> + * 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];
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
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
02.06.2017 15:55, Vladimir Sementsov-Ogievskiy wrote:
> 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...
changed to "-errno on error"
>
>
>>
>>> + * 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
© 2016 - 2026 Red Hat, Inc.