The NBD spec permits including a human-readable error string if
structured replies are in force, so we might as well send the
client the message that we logged on any error.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 22 ++++++++++++++++------
nbd/trace-events | 2 +-
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 23dc6be708..8085d79076 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1289,23 +1289,25 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
uint64_t handle,
uint32_t error,
+ 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)},
- /* FIXME: Support human-readable error message */
+ {.iov_base = msg, .iov_len = strlen(msg)},
};
assert(nbd_err);
- trace_nbd_co_send_structured_error(handle, nbd_err);
+ trace_nbd_co_send_structured_error(handle, nbd_err,
+ nbd_err_lookup(nbd_err), msg);
set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
sizeof(chunk) - sizeof(chunk.h));
stl_be_p(&chunk.error, nbd_err);
- stw_be_p(&chunk.message_length, 0);
+ stw_be_p(&chunk.message_length, iov[1].iov_len);
- return nbd_co_send_iov(client, iov, 1, errp);
+ return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
}
/* nbd_co_receive_request
@@ -1406,6 +1408,7 @@ static coroutine_fn void nbd_trip(void *opaque)
int flags;
int reply_data_len = 0;
Error *local_err = NULL;
+ char *msg = NULL;
trace_nbd_trip();
if (client->closing) {
@@ -1521,14 +1524,20 @@ reply:
if (local_err) {
/* If we get here, local_err was not a fatal error, and should be sent
* to the client. */
+ assert(ret < 0);
+ msg = g_strdup(error_get_pretty(local_err));
error_report_err(local_err);
local_err = NULL;
}
- if (client->structured_reply && request.type == NBD_CMD_READ) {
+ if (client->structured_reply &&
+ (ret < 0 || request.type == NBD_CMD_READ)) {
if (ret < 0) {
+ if (!msg) {
+ msg = g_strdup("");
+ }
ret = nbd_co_send_structured_error(req->client, request.handle,
- -ret, &local_err);
+ -ret, msg, &local_err);
} else {
ret = nbd_co_send_structured_read(req->client, request.handle,
request.from, req->data,
@@ -1539,6 +1548,7 @@ reply:
ret < 0 ? -ret : 0,
req->data, reply_data_len, &local_err);
}
+ g_free(msg);
if (ret < 0) {
error_prepend(&local_err, "Failed to send reply: ");
goto disconnect;
diff --git a/nbd/trace-events b/nbd/trace-events
index 15a9294445..880f211c07 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -57,7 +57,7 @@ nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients
nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n"
nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d"
nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data, size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len = %zu"
-nbd_co_send_structured_error(uint64_t handle, int err) "Send structured error reply: handle = %" PRIu64 ", error = %d"
+nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'"
nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32
nbd_co_receive_request_cmd_write(uint32_t len) "Reading %" PRIu32 " byte(s)"
--
2.13.6
15.10.2017 04:01, Eric Blake wrote:
> The NBD spec permits including a human-readable error string if
> structured replies are in force, so we might as well send the
> client the message that we logged on any error.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> nbd/server.c | 22 ++++++++++++++++------
> nbd/trace-events | 2 +-
> 2 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index 23dc6be708..8085d79076 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1289,23 +1289,25 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
> static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
> uint64_t handle,
> uint32_t error,
> + char *msg,
it's not const because we want to put it into iov..
may be it is better to make it const and convert to (char *) like in
qio_channel_write_all, to
1: more native message parameter type
2: allow constat strings like nbd_co_send_structured_error(..., "some
error")
> Error **errp)
> {
> NBDStructuredError chunk;
> int nbd_err = system_errno_to_nbd_errno(error);
> struct iovec iov[] = {
> {.iov_base = &chunk, .iov_len = sizeof(chunk)},
> - /* FIXME: Support human-readable error message */
> + {.iov_base = msg, .iov_len = strlen(msg)},
> };
msg is always non-zero? so we don't want to send errors without
message.. (1)
>
> assert(nbd_err);
> - trace_nbd_co_send_structured_error(handle, nbd_err);
> + trace_nbd_co_send_structured_error(handle, nbd_err,
> + nbd_err_lookup(nbd_err), msg);
it doesn't really matter, but "nbd_err_lookup(nbd_err)" is a bit
unrelated and looks like part of previous patch
> set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
> sizeof(chunk) - sizeof(chunk.h));
> stl_be_p(&chunk.error, nbd_err);
> - stw_be_p(&chunk.message_length, 0);
> + stw_be_p(&chunk.message_length, iov[1].iov_len);
>
> - return nbd_co_send_iov(client, iov, 1, errp);
> + return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
but you allow messages of zero length..
it's ok, but looks a bit strange in connection with (1)
> }
>
> /* nbd_co_receive_request
> @@ -1406,6 +1408,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> int flags;
> int reply_data_len = 0;
> Error *local_err = NULL;
> + char *msg = NULL;
>
> trace_nbd_trip();
> if (client->closing) {
> @@ -1521,14 +1524,20 @@ reply:
> if (local_err) {
> /* If we get here, local_err was not a fatal error, and should be sent
> * to the client. */
> + assert(ret < 0);
> + msg = g_strdup(error_get_pretty(local_err));
> error_report_err(local_err);
> local_err = NULL;
> }
>
> - if (client->structured_reply && request.type == NBD_CMD_READ) {
> + if (client->structured_reply &&
> + (ret < 0 || request.type == NBD_CMD_READ)) {
> if (ret < 0) {
> + if (!msg) {
> + msg = g_strdup("");
I'd prefer to handle msg=NULL in nbd_co_send_structured_error instead of
this.
> + }
> ret = nbd_co_send_structured_error(req->client, request.handle,
> - -ret, &local_err);
> + -ret, msg, &local_err);
> } else {
> ret = nbd_co_send_structured_read(req->client, request.handle,
> request.from, req->data,
> @@ -1539,6 +1548,7 @@ reply:
> ret < 0 ? -ret : 0,
> req->data, reply_data_len, &local_err);
> }
> + g_free(msg);
> if (ret < 0) {
> error_prepend(&local_err, "Failed to send reply: ");
> goto disconnect;
> diff --git a/nbd/trace-events b/nbd/trace-events
> index 15a9294445..880f211c07 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -57,7 +57,7 @@ nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients
> nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n"
> nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d"
> nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data, size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len = %zu"
> -nbd_co_send_structured_error(uint64_t handle, int err) "Send structured error reply: handle = %" PRIu64 ", error = %d"
> +nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'"
> nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
> nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32
> nbd_co_receive_request_cmd_write(uint32_t len) "Reading %" PRIu32 " byte(s)"
anyway it should work, so, with or without my suggestions:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
On 10/16/2017 05:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2017 04:01, Eric Blake wrote:
>> The NBD spec permits including a human-readable error string if
>> structured replies are in force, so we might as well send the
>> client the message that we logged on any error.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> nbd/server.c | 22 ++++++++++++++++------
>> nbd/trace-events | 2 +-
>> 2 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 23dc6be708..8085d79076 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -1289,23 +1289,25 @@ static int coroutine_fn
>> nbd_co_send_structured_read(NBDClient *client,
>> static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
>> uint64_t handle,
>> uint32_t error,
>> + char *msg,
>
> it's not const because we want to put it into iov..
> may be it is better to make it const and convert to (char *) like in
> qio_channel_write_all, to
> 1: more native message parameter type
> 2: allow constat strings like nbd_co_send_structured_error(..., "some
> error")
Yes, casting away const would be a more convenient interface (it's a
bummer that iov can't be const-correct for write, because it is also
used by read which cannot be const).
>
>> Error **errp)
>> {
>> NBDStructuredError chunk;
>> int nbd_err = system_errno_to_nbd_errno(error);
>> struct iovec iov[] = {
>> {.iov_base = &chunk, .iov_len = sizeof(chunk)},
>> - /* FIXME: Support human-readable error message */
>> + {.iov_base = msg, .iov_len = strlen(msg)},
>> };
>
> msg is always non-zero? so we don't want to send errors without
> message.. (1)
I played with the idea of allowing NULL, and don't know whether it was
easier to require non-NULL message (which may require NULL check at all
callers) or to allow NULL (requiring more code here).
>
>>
>> assert(nbd_err);
>> - trace_nbd_co_send_structured_error(handle, nbd_err);
>> + trace_nbd_co_send_structured_error(handle, nbd_err,
>> + nbd_err_lookup(nbd_err), msg);
>
> it doesn't really matter, but "nbd_err_lookup(nbd_err)" is a bit
> unrelated and looks like part of previous patch
The previous patch was yours; I tried to minimize the changes I made to
your patches, so I stuck this one in mine instead. But if you think we
should trace accurately from the get-go, I'm just fine rebasing the
series to make your patch trace the error name at its introduction,
rather than in my followup that supports error messages.
>
>> set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE,
>> NBD_REPLY_TYPE_ERROR, handle,
>> sizeof(chunk) - sizeof(chunk.h));
>> stl_be_p(&chunk.error, nbd_err);
>> - stw_be_p(&chunk.message_length, 0);
>> + stw_be_p(&chunk.message_length, iov[1].iov_len);
>>
>> - return nbd_co_send_iov(client, iov, 1, errp);
>> + return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
>
> but you allow messages of zero length..
> it's ok, but looks a bit strange in connection with (1)
Passing "" is the easiest thing to do for callers that can't pass NULL.
My first attempt had:
.iov_len = msg ? strlen(msg) : 0
and
trace_...(msg ? msg : "")
For this patch, there is only a single caller, so it's really hard to
judge which style (required non-NULL parameter, vs. doing more work in
the common function) will be more useful until we add further callers.
>> - if (client->structured_reply && request.type == NBD_CMD_READ) {
>> + if (client->structured_reply &&
>> + (ret < 0 || request.type == NBD_CMD_READ)) {
>> if (ret < 0) {
>> + if (!msg) {
>> + msg = g_strdup("");
>
> I'd prefer to handle msg=NULL in nbd_co_send_structured_error instead of
> this.
Okay, then it sounds like accepting a NULL parameter is the way to go!
It's also worth considering that casting away const would mean I don't
have to g_strdup("").
> anyway it should work, so, with or without my suggestions:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Thanks for your reviews!
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On 10/14/2017 08:01 PM, Eric Blake wrote:
> The NBD spec permits including a human-readable error string if
> structured replies are in force, so we might as well send the
> client the message that we logged on any error.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> nbd/server.c | 22 ++++++++++++++++------
> nbd/trace-events | 2 +-
> 2 files changed, 17 insertions(+), 7 deletions(-)
>
> assert(nbd_err);
> - trace_nbd_co_send_structured_error(handle, nbd_err);
> + trace_nbd_co_send_structured_error(handle, nbd_err,
> + nbd_err_lookup(nbd_err), msg);
> set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
> sizeof(chunk) - sizeof(chunk.h));
Bug - it's a bad idea to not include the message length in the overall
length, because the client then gets out of sync with the server (it
reads only 6 bytes instead of 6+strlen(msg) bytes, and expects the
message to start with the magic number for the next reply).
> stl_be_p(&chunk.error, nbd_err);
> - stw_be_p(&chunk.message_length, 0);
> + stw_be_p(&chunk.message_length, iov[1].iov_len);
But this also highlights a bug in 9/8, where we have:
> +static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk,
> + uint8_t *payload, int *request_ret,
> + Error **errp)
> +{
> + uint32_t error;
> + uint16_t message_size;
> +
> + assert(chunk->type & (1 << 15));
> +
> + if (chunk->length < sizeof(error) + sizeof(message_size)) {
> + error_setg(errp,
> + "Protocol error: invalid payload for structured error");
> + return -EINVAL;
> + }
> +
> + error = nbd_errno_to_system_errno(payload_advance32(&payload));
> + if (error == 0) {
> + error_setg(errp, "Protocol error: server sent structured error chunk"
> + "with error = 0");
> + return -EINVAL;
> + }
> +
> + *request_ret = error;
> + message_size = payload_advance16(&payload);
> + error_setg_errno(errp, error, "%.*s", message_size, payload);
Whoops - no sanity check that message_size fits within chunk->length.
So when we read message_length 33 (when the server sends a message 33
bytes long), we are then dereferencing up to 33 bytes of garbage beyond
the end of payload.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
© 2016 - 2026 Red Hat, Inc.