Re: [Qemu-block] [PATCH for-2.12] nbd: trace meta context negotiation

[Eric Blake]

On 03/30/2018 11:32 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.03.2018 16:09, Eric Blake wrote:
>> Having a more detailed log of the interaction between client and
>> server is invaluable in debugging how meta context negotiation
>> actually works.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>   nbd/client.c     | 2 ++
>>   nbd/server.c     | 8 ++++++++
>>   nbd/trace-events | 6 ++++++
>>   3 files changed, 16 insertions(+)
>>

>> @@ -800,6 +804,7 @@ static int nbd_negotiate_meta_query(NBDClient
>> *client,
>>       /* The only supported namespace for now is 'base'. So query
>> should start
>>        * with 'base:'. Otherwise, we can ignore it and skip the
>> remainder. */
>>       if (len < baselen) {
>> +        trace_nbd_negotiate_meta_query_skip("length too short");
>>           return nbd_opt_skip(client, len, errp);
>>       }
>>
>> @@ -809,6 +814,7 @@ static int nbd_negotiate_meta_query(NBDClient
>> *client,
>>           return ret;
>>       }
>>       if (strncmp(query, "base:", baselen) != 0) {
>> +        trace_nbd_negotiate_meta_query_skip("not for base: namespace");
> 
> Hmm, reasonable example of using same trace-point in several places in
> the code. Is it a precedent?

Yes, trace points have been reused before; as long as the parameters are
the same and the message is reasonable, the only reason to NOT share a
trace is if you ever see yourself needing to select between the two
trace points individually. But for both of these points, it seems to me
that you'd either be tracing everything nbd_* or nothing, rather than
trying to catch just one of these points.

>> +++ b/nbd/trace-events
>> @@ -10,6 +10,8 @@ nbd_receive_query_exports_start(const char
>> *wantname) "Querying export list for
>>   nbd_receive_query_exports_success(const char *wantname) "Found
>> desired export name '%s'"
>>   nbd_receive_starttls_new_client(void) "Setting up TLS"
>>   nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
>> +nbd_opt_meta_request(const char *context, const char *export)
>> "Requesting to set meta context %s for export %s"
>> +nbd_opt_meta_reply(const char *context, int id) "Received mapping of
>> context %s to id %d"
> 
> actual type is uint32_t, I'd prefer to reflect it here.
> 

It doesn't make a difference on 'unsigned int' vs. 'uint32_t' in
trace-events.  We guarantee that int is 32 bits on all platforms qemu
compiles on, and "%u" is a lot less typing than "%"PRIu32.  But you are
right that id is unsigned, and printing a negative value for an id could
be confusing, so I'll at least tweak it for that.

> 
> Must-have trace-points, anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

Thanks; I'll make those trace-events tweaks, and queue in my NBD tree
for a pull request today.

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

signature.asc
Description: OpenPGP digital signature