[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v4 5/8] nbd/server: Include human-readable messa
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors |
Date: |
Mon, 16 Oct 2017 07:26:08 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
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 <address@hidden>
>> ---
>> 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 <address@hidden>
Thanks for your reviews!
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v4 3/8] nbd: Expose constants and structs for structured read, (continued)
- [Qemu-block] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors, Eric Blake, 2017/10/14
- [Qemu-block] [PATCH v4 6/8] nbd/client: refactor nbd_receive_starttls, Eric Blake, 2017/10/14
- [Qemu-block] [PATCH v4 8/8] nbd: Move nbd_read() to common header, Eric Blake, 2017/10/14
- [Qemu-block] [PATCH v4 7/8] nbd/client: prepare nbd_receive_reply for structured reply, Eric Blake, 2017/10/14
- [Qemu-block] [PATCH v4 RFC 9/8] nbd: Minimal structured read for client, Vladimir Sementsov-Ogievskiy, 2017/10/17