Re: [Qemu-devel] [PATCH v3 04/13] nbd/server: structurize simple reply header sending

[Eric Blake]

On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> Use packed structure instead of pointer arithmetics.

English is fun!  In the subject line, I'm fairly certain that
"structurize" is not likely to be in any dictionary, yet it is a perfect
word describing the patch, so I'm not touching it ;)

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  include/block/nbd.h |  6 ++++++
>  nbd/server.c        | 36 ++++++++++++++----------------------
>  2 files changed, 20 insertions(+), 22 deletions(-)
> 

> +++ b/nbd/server.c
> @@ -902,26 +902,6 @@ static int nbd_receive_request(QIOChannel *ioc, 
> NBDRequest *request,
>      return 0;
>  }
>  
> -static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
> -{
> -    uint8_t buf[NBD_REPLY_SIZE];
> -
> -    reply->error = system_errno_to_nbd_errno(reply->error);
> -
> -    trace_nbd_send_reply(reply->error, reply->handle);

We lose a trace here...

>  static int nbd_co_send_simple_reply(NBDRequestData *req, NBDReply *reply,
>                                      int len, Error **errp)
>  {
>      NBDClient *client = req->client;
> +    NBDSimpleReply simple_reply;
>      int ret;
>  
>      g_assert(qemu_in_coroutine());
>  
>      trace_nbd_co_send_simple_reply(reply->handle, reply->error, len);
>  
> +    set_be_simple_reply(&simple_reply, 
> system_errno_to_nbd_errno(reply->error),
> +                        reply->handle);
> +

...but it always occurred immediately after another trace that has
redundant information (well, the trace you kept shows pre- rather than
post-translation of errno value to NBD wire value, and the trace you
drop didn't show length).  I'm fine with the reduction, but it needs a
tweak to trace-events to reap the dead trace.

With that change,
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature