Re: [Qemu-devel] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors

[Eric Blake]

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