* [Qemu-devel] [PATCH] nbd/server: fix space read
@ 2018-03-05 18:04 Vladimir Sementsov-Ogievskiy
2018-03-05 19:47 ` Eric Blake
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-03-05 18:04 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: pbonzini, eblake, vsementsov, den
In case of io error in nbd_co_send_sparse_read we should not
"goto reply:", as it is fatal error and common behavior is
disconnect in this case. We should not try to send client an
error reply, representing channel-io error on previous try to
send a reply.
Fix this by handle block-status error in nbd_co_send_sparse_read,
so nbd_co_send_sparse_read fails only on io error. Then just skip
common "reply:" code path in nbd_trip.
Note: nbd_co_send_structured_error is moved without changes to be
called from nbd_co_send_sparse_read.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
PS: this all shows that nbd_trip is tooo complex (and yes, I remember,
that its final semantics was made by myself :(.. It should be
refactored into several smaller parts. Do you have any ideas?
The complexity here is that we should handle channel errors (fatal) and
export errors(non fatal) in different ways, and both of error types may
have errp, which should be handled differently too..
May be, the best way is to make separate functions for each command,
avoiding code duplication by using helper-functions instead of common code
in nbd_trip.
nbd/server.c | 64 +++++++++++++++++++++++++++++++++++-------------------------
1 file changed, 37 insertions(+), 27 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 4990a5826e..ea6b9467e4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -21,6 +21,7 @@
#include "qapi/error.h"
#include "trace.h"
#include "nbd-internal.h"
+#include "qemu/error-report.h"
static int system_errno_to_nbd_errno(int err)
{
@@ -1341,6 +1342,30 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
return nbd_co_send_iov(client, iov, 2, errp);
}
+static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
+ uint64_t handle,
+ uint32_t error,
+ const char *msg,
+ Error **errp)
+{
+ NBDStructuredError chunk;
+ int nbd_err = system_errno_to_nbd_errno(error);
+ struct iovec iov[] = {
+ {.iov_base = &chunk, .iov_len = sizeof(chunk)},
+ {.iov_base = (char *)msg, .iov_len = msg ? strlen(msg) : 0},
+ };
+
+ assert(nbd_err);
+ trace_nbd_co_send_structured_error(handle, nbd_err,
+ nbd_err_lookup(nbd_err), msg ? msg : "");
+ set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
+ sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
+ stl_be_p(&chunk.error, nbd_err);
+ stw_be_p(&chunk.message_length, iov[1].iov_len);
+
+ return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
+}
+
static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
uint64_t handle,
uint64_t offset,
@@ -1361,8 +1386,15 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
bool final;
if (status < 0) {
- error_setg_errno(errp, -status, "unable to check for holes");
- return status;
+ char *msg = g_strdup_printf("unable to check for holes: %s",
+ strerror(-status));
+
+ error_report("%s", msg);
+
+ ret = nbd_co_send_structured_error(client, handle, -status, msg,
+ errp);
+ g_free(msg);
+ return ret;
}
assert(pnum && pnum <= size - progress);
final = progress + pnum == size;
@@ -1400,30 +1432,6 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
return ret;
}
-static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
- uint64_t handle,
- uint32_t error,
- const char *msg,
- Error **errp)
-{
- NBDStructuredError chunk;
- int nbd_err = system_errno_to_nbd_errno(error);
- struct iovec iov[] = {
- {.iov_base = &chunk, .iov_len = sizeof(chunk)},
- {.iov_base = (char *)msg, .iov_len = msg ? strlen(msg) : 0},
- };
-
- assert(nbd_err);
- trace_nbd_co_send_structured_error(handle, nbd_err,
- nbd_err_lookup(nbd_err), msg ? msg : "");
- set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
- sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
- stl_be_p(&chunk.error, nbd_err);
- stw_be_p(&chunk.message_length, iov[1].iov_len);
-
- return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
-}
-
/* nbd_co_receive_request
* Collect a client request. Return 0 if request looks valid, -EIO to drop
* connection right away, and any other negative value to report an error to
@@ -1567,7 +1575,7 @@ static coroutine_fn void nbd_trip(void *opaque)
request.from, req->data, request.len,
&local_err);
if (ret < 0) {
- goto reply;
+ goto replied;
}
goto done;
}
@@ -1664,6 +1672,8 @@ reply:
req->data, reply_data_len, &local_err);
}
g_free(msg);
+
+replied:
if (ret < 0) {
error_prepend(&local_err, "Failed to send reply: ");
goto disconnect;
--
2.11.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] nbd/server: fix space read
2018-03-05 18:04 [Qemu-devel] [PATCH] nbd/server: fix space read Vladimir Sementsov-Ogievskiy
@ 2018-03-05 19:47 ` Eric Blake
2018-03-05 19:56 ` Eric Blake
2018-03-08 11:50 ` Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 6+ messages in thread
From: Eric Blake @ 2018-03-05 19:47 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block; +Cc: pbonzini, den
On 03/05/2018 12:04 PM, Vladimir Sementsov-Ogievskiy wrote:
> In case of io error in nbd_co_send_sparse_read we should not
> "goto reply:", as it is fatal error and common behavior is
> disconnect in this case. We should not try to send client an
> error reply, representing channel-io error on previous try to
> send a reply.
Good catch.
>
> Fix this by handle block-status error in nbd_co_send_sparse_read,
> so nbd_co_send_sparse_read fails only on io error. Then just skip
> common "reply:" code path in nbd_trip.
>
> Note: nbd_co_send_structured_error is moved without changes to be
> called from nbd_co_send_sparse_read.
Might be easier to read as two patches, one for the code motion, the
other for using the new code. But I'm not going to insist on a split.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> PS: this all shows that nbd_trip is tooo complex (and yes, I remember,
> that its final semantics was made by myself :(.. It should be
> refactored into several smaller parts. Do you have any ideas?
>
> The complexity here is that we should handle channel errors (fatal) and
> export errors(non fatal) in different ways, and both of error types may
> have errp, which should be handled differently too..
>
> May be, the best way is to make separate functions for each command,
> avoiding code duplication by using helper-functions instead of common code
> in nbd_trip.
Yes, splitting nbd_trip into smaller helper functions may be worthwhile.
>
> nbd/server.c | 64 +++++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index 4990a5826e..ea6b9467e4 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -21,6 +21,7 @@
> #include "qapi/error.h"
> #include "trace.h"
> #include "nbd-internal.h"
> +#include "qemu/error-report.h"
I'm a bit worried about this one. The server has previously not needed
this, so it may be the wrong thing to pull it in now.
> +
> static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
> uint64_t handle,
> uint64_t offset,
It doesn't help that we are lacking comments on the contract of this
function.
> @@ -1361,8 +1386,15 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
> bool final;
>
> if (status < 0) {
> - error_setg_errno(errp, -status, "unable to check for holes");
> - return status;
> + char *msg = g_strdup_printf("unable to check for holes: %s",
> + strerror(-status));
Indentation looks off.
> +
> + error_report("%s", msg);
Do we really need to report this on the server's stderr, or is sending
the reply to the client good enough?
> +
> + ret = nbd_co_send_structured_error(client, handle, -status, msg,
> + errp);
> + g_free(msg);
> + return ret;
So if we have an early return here, then pre-patch, we were
unconditionally setting errp and returning -1 (even if the client is
still alive); post-patch errp is only set if we failed to do I/O with
the client. That change is right, but harder to see without comments
giving a function contract.
> @@ -1567,7 +1575,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> request.from, req->data, request.len,
> &local_err);
> if (ret < 0) {
> - goto reply;
> + goto replied;
> }
> goto done;
> }
> @@ -1664,6 +1672,8 @@ reply:
> req->data, reply_data_len, &local_err);
> }
> g_free(msg);
> +
> +replied:
This part makes sense.
> if (ret < 0) {
> error_prepend(&local_err, "Failed to send reply: ");
> goto disconnect;
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] nbd/server: fix space read
2018-03-05 19:47 ` Eric Blake
@ 2018-03-05 19:56 ` Eric Blake
2018-03-08 11:50 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 6+ messages in thread
From: Eric Blake @ 2018-03-05 19:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block; +Cc: pbonzini, den
On 03/05/2018 01:47 PM, Eric Blake wrote:
> On 03/05/2018 12:04 PM, Vladimir Sementsov-Ogievskiy wrote:
In the subject line: s/space/sparse/
>> In case of io error in nbd_co_send_sparse_read we should not
>> "goto reply:", as it is fatal error and common behavior is
>> disconnect in this case. We should not try to send client an
>> error reply, representing channel-io error on previous try to
>> send a reply.
>
> Good catch.
>
>>
>> Fix this by handle block-status error in nbd_co_send_sparse_read,
>> so nbd_co_send_sparse_read fails only on io error. Then just skip
>> common "reply:" code path in nbd_trip.
>>
>> Note: nbd_co_send_structured_error is moved without changes to be
>> called from nbd_co_send_sparse_read.
>
> Might be easier to read as two patches, one for the code motion, the
> other for using the new code. But I'm not going to insist on a split.
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] nbd/server: fix space read
2018-03-05 19:47 ` Eric Blake
2018-03-05 19:56 ` Eric Blake
@ 2018-03-08 11:50 ` Vladimir Sementsov-Ogievskiy
2018-03-08 15:17 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-03-08 11:50 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block; +Cc: pbonzini, den
05.03.2018 22:47, Eric Blake wrote:
> On 03/05/2018 12:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>> In case of io error in nbd_co_send_sparse_read we should not
>> "goto reply:", as it is fatal error and common behavior is
>> disconnect in this case. We should not try to send client an
>> error reply, representing channel-io error on previous try to
>> send a reply.
>
> Good catch.
>
>>
>> Fix this by handle block-status error in nbd_co_send_sparse_read,
>> so nbd_co_send_sparse_read fails only on io error. Then just skip
>> common "reply:" code path in nbd_trip.
>>
>> Note: nbd_co_send_structured_error is moved without changes to be
>> called from nbd_co_send_sparse_read.
>
> Might be easier to read as two patches, one for the code motion, the
> other for using the new code. But I'm not going to insist on a split.
>
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> PS: this all shows that nbd_trip is tooo complex (and yes, I remember,
>> that its final semantics was made by myself :(.. It should be
>> refactored into several smaller parts. Do you have any ideas?
>>
>> The complexity here is that we should handle channel errors (fatal) and
>> export errors(non fatal) in different ways, and both of error types may
>> have errp, which should be handled differently too..
>>
>> May be, the best way is to make separate functions for each command,
>> avoiding code duplication by using helper-functions instead of common
>> code
>> in nbd_trip.
>
> Yes, splitting nbd_trip into smaller helper functions may be worthwhile.
>
>>
>> nbd/server.c | 64
>> +++++++++++++++++++++++++++++++++++-------------------------
>> 1 file changed, 37 insertions(+), 27 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 4990a5826e..ea6b9467e4 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -21,6 +21,7 @@
>> #include "qapi/error.h"
>> #include "trace.h"
>> #include "nbd-internal.h"
>> +#include "qemu/error-report.h"
>
> I'm a bit worried about this one. The server has previously not
> needed this, so it may be the wrong thing to pull it in now.
>
>
>> +
>> static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
>> uint64_t handle,
>> uint64_t offset,
>
> It doesn't help that we are lacking comments on the contract of this
> function.
>
>> @@ -1361,8 +1386,15 @@ static int coroutine_fn
>> nbd_co_send_sparse_read(NBDClient *client,
>> bool final;
>> if (status < 0) {
>> - error_setg_errno(errp, -status, "unable to check for
>> holes");
>> - return status;
>> + char *msg = g_strdup_printf("unable to check for holes:
>> %s",
>> + strerror(-status));
>
> Indentation looks off.
>
>> +
>> + error_report("%s", msg);
>
> Do we really need to report this on the server's stderr, or is sending
> the reply to the client good enough?
I'm just trying to save current behavior in nbd_trip... Why not? Is it
fair that client knows about some strange io error on server-side, but
server logs lacks this information?
and it needs "qemu/error-report.h".. nbd_trip uses error_report_err
instead. error_report_err calls error_report anyway.
>
>> +
>> + ret = nbd_co_send_structured_error(client, handle,
>> -status, msg,
>> + errp);
>> + g_free(msg);
>> + return ret;
>
> So if we have an early return here, then pre-patch, we were
> unconditionally setting errp and returning -1 (even if the client is
> still alive); post-patch errp is only set if we failed to do I/O with
> the client. That change is right, but harder to see without comments
> giving a function contract.
>
>> @@ -1567,7 +1575,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>> request.from, req->data,
>> request.len,
>> &local_err);
>> if (ret < 0) {
>> - goto reply;
>> + goto replied;
>> }
>> goto done;
>> }
>> @@ -1664,6 +1672,8 @@ reply:
>> req->data, reply_data_len,
>> &local_err);
>> }
>> g_free(msg);
>> +
>> +replied:
>
> This part makes sense.
>
>> if (ret < 0) {
>> error_prepend(&local_err, "Failed to send reply: ");
>> goto disconnect;
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] nbd/server: fix space read
2018-03-08 11:50 ` Vladimir Sementsov-Ogievskiy
@ 2018-03-08 15:17 ` Vladimir Sementsov-Ogievskiy
2018-03-08 15:20 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-03-08 15:17 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block; +Cc: pbonzini, den
08.03.2018 14:50, Vladimir Sementsov-Ogievskiy wrote:
> 05.03.2018 22:47, Eric Blake wrote:
>> On 03/05/2018 12:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> In case of io error in nbd_co_send_sparse_read we should not
>>> "goto reply:", as it is fatal error and common behavior is
>>> disconnect in this case. We should not try to send client an
>>> error reply, representing channel-io error on previous try to
>>> send a reply.
>>
>> Good catch.
>>
>>>
>>> Fix this by handle block-status error in nbd_co_send_sparse_read,
>>> so nbd_co_send_sparse_read fails only on io error. Then just skip
>>> common "reply:" code path in nbd_trip.
>>>
>>> Note: nbd_co_send_structured_error is moved without changes to be
>>> called from nbd_co_send_sparse_read.
>>
>> Might be easier to read as two patches, one for the code motion, the
>> other for using the new code. But I'm not going to insist on a split.
>>
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>
>>> PS: this all shows that nbd_trip is tooo complex (and yes, I remember,
>>> that its final semantics was made by myself :(.. It should be
>>> refactored into several smaller parts. Do you have any ideas?
>>>
>>> The complexity here is that we should handle channel errors (fatal) and
>>> export errors(non fatal) in different ways, and both of error types may
>>> have errp, which should be handled differently too..
>>>
>>> May be, the best way is to make separate functions for each command,
>>> avoiding code duplication by using helper-functions instead of
>>> common code
>>> in nbd_trip.
>>
>> Yes, splitting nbd_trip into smaller helper functions may be worthwhile.
>>
>>>
>>> nbd/server.c | 64
>>> +++++++++++++++++++++++++++++++++++-------------------------
>>> 1 file changed, 37 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/nbd/server.c b/nbd/server.c
>>> index 4990a5826e..ea6b9467e4 100644
>>> --- a/nbd/server.c
>>> +++ b/nbd/server.c
>>> @@ -21,6 +21,7 @@
>>> #include "qapi/error.h"
>>> #include "trace.h"
>>> #include "nbd-internal.h"
>>> +#include "qemu/error-report.h"
>>
>> I'm a bit worried about this one. The server has previously not
>> needed this, so it may be the wrong thing to pull it in now.
>>
>>
>>> +
>>> static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
>>> uint64_t handle,
>>> uint64_t offset,
>>
>> It doesn't help that we are lacking comments on the contract of this
>> function.
>>
>>> @@ -1361,8 +1386,15 @@ static int coroutine_fn
>>> nbd_co_send_sparse_read(NBDClient *client,
>>> bool final;
>>> if (status < 0) {
>>> - error_setg_errno(errp, -status, "unable to check for
>>> holes");
>>> - return status;
>>> + char *msg = g_strdup_printf("unable to check for holes:
>>> %s",
>>> + strerror(-status));
>>
>> Indentation looks off.
>>
>>> +
>>> + error_report("%s", msg);
>>
>> Do we really need to report this on the server's stderr, or is
>> sending the reply to the client good enough?
>
>
> I'm just trying to save current behavior in nbd_trip... Why not? Is it
> fair that client knows about some strange io error on server-side, but
> server logs lacks this information?
May be, it worth switching it from error_report to trace, but it should
be a separate patch, changing all places around.
>
> and it needs "qemu/error-report.h".. nbd_trip uses error_report_err
> instead. error_report_err calls error_report anyway.
>
>>
>>> +
>>> + ret = nbd_co_send_structured_error(client, handle,
>>> -status, msg,
>>> + errp);
>>> + g_free(msg);
>>> + return ret;
>>
>> So if we have an early return here, then pre-patch, we were
>> unconditionally setting errp and returning -1 (even if the client is
>> still alive); post-patch errp is only set if we failed to do I/O with
>> the client. That change is right, but harder to see without comments
>> giving a function contract.
>>
>>> @@ -1567,7 +1575,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>>> request.from, req->data,
>>> request.len,
>>> &local_err);
>>> if (ret < 0) {
>>> - goto reply;
>>> + goto replied;
>>> }
>>> goto done;
>>> }
>>> @@ -1664,6 +1672,8 @@ reply:
>>> req->data, reply_data_len,
>>> &local_err);
>>> }
>>> g_free(msg);
>>> +
>>> +replied:
>>
>> This part makes sense.
>>
>>> if (ret < 0) {
>>> error_prepend(&local_err, "Failed to send reply: ");
>>> goto disconnect;
>>>
>>
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] nbd/server: fix space read
2018-03-08 15:17 ` Vladimir Sementsov-Ogievskiy
@ 2018-03-08 15:20 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-03-08 15:20 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block; +Cc: pbonzini, den
08.03.2018 18:17, Vladimir Sementsov-Ogievskiy wrote:
> 08.03.2018 14:50, Vladimir Sementsov-Ogievskiy wrote:
>> 05.03.2018 22:47, Eric Blake wrote:
>>> On 03/05/2018 12:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> In case of io error in nbd_co_send_sparse_read we should not
>>>> "goto reply:", as it is fatal error and common behavior is
>>>> disconnect in this case. We should not try to send client an
>>>> error reply, representing channel-io error on previous try to
>>>> send a reply.
>>>
>>> Good catch.
>>>
>>>>
>>>> Fix this by handle block-status error in nbd_co_send_sparse_read,
>>>> so nbd_co_send_sparse_read fails only on io error. Then just skip
>>>> common "reply:" code path in nbd_trip.
>>>>
>>>> Note: nbd_co_send_structured_error is moved without changes to be
>>>> called from nbd_co_send_sparse_read.
>>>
>>> Might be easier to read as two patches, one for the code motion, the
>>> other for using the new code. But I'm not going to insist on a split.
>>>
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>
>>>> PS: this all shows that nbd_trip is tooo complex (and yes, I remember,
>>>> that its final semantics was made by myself :(.. It should be
>>>> refactored into several smaller parts. Do you have any ideas?
>>>>
>>>> The complexity here is that we should handle channel errors (fatal)
>>>> and
>>>> export errors(non fatal) in different ways, and both of error types
>>>> may
>>>> have errp, which should be handled differently too..
>>>>
>>>> May be, the best way is to make separate functions for each command,
>>>> avoiding code duplication by using helper-functions instead of
>>>> common code
>>>> in nbd_trip.
>>>
>>> Yes, splitting nbd_trip into smaller helper functions may be
>>> worthwhile.
>>>
>>>>
>>>> nbd/server.c | 64
>>>> +++++++++++++++++++++++++++++++++++-------------------------
>>>> 1 file changed, 37 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/nbd/server.c b/nbd/server.c
>>>> index 4990a5826e..ea6b9467e4 100644
>>>> --- a/nbd/server.c
>>>> +++ b/nbd/server.c
>>>> @@ -21,6 +21,7 @@
>>>> #include "qapi/error.h"
>>>> #include "trace.h"
>>>> #include "nbd-internal.h"
>>>> +#include "qemu/error-report.h"
>>>
>>> I'm a bit worried about this one. The server has previously not
>>> needed this, so it may be the wrong thing to pull it in now.
>>>
>>>
>>>> +
>>>> static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
>>>> uint64_t handle,
>>>> uint64_t offset,
>>>
>>> It doesn't help that we are lacking comments on the contract of this
>>> function.
>>>
>>>> @@ -1361,8 +1386,15 @@ static int coroutine_fn
>>>> nbd_co_send_sparse_read(NBDClient *client,
>>>> bool final;
>>>> if (status < 0) {
>>>> - error_setg_errno(errp, -status, "unable to check for
>>>> holes");
>>>> - return status;
>>>> + char *msg = g_strdup_printf("unable to check for
>>>> holes: %s",
>>>> + strerror(-status));
>>>
>>> Indentation looks off.
>>>
>>>> +
>>>> + error_report("%s", msg);
>>>
>>> Do we really need to report this on the server's stderr, or is
>>> sending the reply to the client good enough?
>>
>>
>> I'm just trying to save current behavior in nbd_trip... Why not? Is
>> it fair that client knows about some strange io error on server-side,
>> but server logs lacks this information?
>
> May be, it worth switching it from error_report to trace, but it
> should be a separate patch, changing all places around.
stop, it is already traced in nbd_co_send_simple_reply and
nbd_co_send_structured_error. So, we can safely drop it.
>
>>
>> and it needs "qemu/error-report.h".. nbd_trip uses error_report_err
>> instead. error_report_err calls error_report anyway.
>>
>>>
>>>> +
>>>> + ret = nbd_co_send_structured_error(client, handle,
>>>> -status, msg,
>>>> + errp);
>>>> + g_free(msg);
>>>> + return ret;
>>>
>>> So if we have an early return here, then pre-patch, we were
>>> unconditionally setting errp and returning -1 (even if the client is
>>> still alive); post-patch errp is only set if we failed to do I/O
>>> with the client. That change is right, but harder to see without
>>> comments giving a function contract.
>>>
>>>> @@ -1567,7 +1575,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>>>> request.from,
>>>> req->data, request.len,
>>>> &local_err);
>>>> if (ret < 0) {
>>>> - goto reply;
>>>> + goto replied;
>>>> }
>>>> goto done;
>>>> }
>>>> @@ -1664,6 +1672,8 @@ reply:
>>>> req->data, reply_data_len,
>>>> &local_err);
>>>> }
>>>> g_free(msg);
>>>> +
>>>> +replied:
>>>
>>> This part makes sense.
>>>
>>>> if (ret < 0) {
>>>> error_prepend(&local_err, "Failed to send reply: ");
>>>> goto disconnect;
>>>>
>>>
>>
>>
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-08 15:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-05 18:04 [Qemu-devel] [PATCH] nbd/server: fix space read Vladimir Sementsov-Ogievskiy
2018-03-05 19:47 ` Eric Blake
2018-03-05 19:56 ` Eric Blake
2018-03-08 11:50 ` Vladimir Sementsov-Ogievskiy
2018-03-08 15:17 ` Vladimir Sementsov-Ogievskiy
2018-03-08 15:20 ` Vladimir Sementsov-Ogievskiy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).