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 <ebl...@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 <vsement...@virtuozzo.com> 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