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 <[email protected]> >> --- >> 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 <[email protected]>
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
