qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCHv13/8] trace: [tcg] Identify events with the 'vcpu' property
Date: Tue, 13 Oct 2015 11:33:38 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

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.

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.

> +++ 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.

>  # Since 2.2
>  ##
>  { 'struct': 'TraceEventInfo',
> -  'data': {'name': 'str', 'state': 'TraceEventState'} }
> +  'data': {'name': 'str', 'state': 'TraceEventState', 'vcpu': 'bool'} }

This is okay from the qapi interface perspective (events are output, so
unconditional addition of new fields won't break existing clients).

I did not closely review the rest, however, I did spot this:

> +++ 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;

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]