Re: [Qemu-devel] [PATCHv13/8] trace: [tcg] Identify events with the 'vcpu' property

[Lluís Vilanova]

Eric Blake writes:

> On 10/13/2015 11:10 AM, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <address@hidden>

> If you'd send with 'qemu format-patch/send-email -v1', then your subject
> line would be formatted [PATCH v1 3/8] instead of the confusing results
> you got by omitting spaces [PATCHv13/8] (this is v13? out of 8?).  Also,
> no need to include v1 (it's fairly obvious that an unversioned patch is
> the first), you really only need the designation for -v2 and beyond.

Right, it's just that "stgit mail" does not add the spaces for you.


> The one-line commit subject correctly explains the 'what', but there is
> no commit body explaining the 'why'.  While a commit body is not
> mandatory, it usually helps.

I've had these lying around for a very long time and now I see I got lazy in
writing the descriptions. Sorry about that.


>> +++ b/qapi/trace.json
>> @@ -1,6 +1,6 @@
>> # -*- mode: python -*-
>> #
>> -# Copyright (C) 2011-2014 Lluís Vilanova <address@hidden>
>> +# Copyright (C) 2011-2015 Lluís Vilanova <address@hidden>
>> #
>> # This work is licensed under the terms of the GNU GPL, version 2 or later.
>> # See the COPYING file in the top-level directory.
>> @@ -29,11 +29,12 @@
>> #
>> # @name: Event name.
>> # @state: Tracing state.
>> +# @vcpu: Whether this is a per-vCPU event.
>> #

> Missing a '(since 2.5)' comment on the @vcpu line.

[...]
>> +++ b/trace/qmp.c
>> @@ -22,6 +22,8 @@ TraceEventInfoList *qmp_trace_event_get_state(const char 
>> *name, Error **errp)
>> while ((ev = trace_event_pattern(name, ev)) != NULL) {
>> TraceEventInfoList *elem = g_new(TraceEventInfoList, 1);
elem-> value = g_new(TraceEventInfo, 1);
>> +        elem->value->vcpu =
>> +            trace_event_get_cpu_id(ev) == TRACE_CPU_EVENT_COUNT ? false : 
>> true;

> I'm not a fan of the over-verbose 'cond ? false : true'.  It can almost
> always be written '!cond' with just as much clarity.  In your case:

> elem-> value->vcpu = trace_event_get_cpu_id(ev) != TRACE_CPU_EVENT_COUNT;

I guess it's a matter of personal taste. I'll add both to version 2.


Thanks,
  Lluis

-- 
"And it's much the same thing with knowledge, for whenever you learn
something new, the whole world becomes that much richer."
-- The Princess of Pure Reason, as told by Norton Juster in The Phantom
Tollbooth