Re: [Qemu-devel] [PATCH v7 7/7] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state

2016-06-30 Thread Lluís Vilanova 
Markus Armbruster writes:

> Lluís Vilanova  writes:
>> Signed-off-by: Lluís Vilanova 
>> Reviewed-by: Stefan Hajnoczi 
>> ---
>> hmp-commands-info.hx |6 +-
>> hmp-commands.hx  |7 +-
>> monitor.c|   17 +-
>> qapi/trace.json  |   32 +--
>> qmp-commands.hx  |   35 +++-
>> trace/qmp.c  |  148 
>> --
>> 6 files changed, 202 insertions(+), 43 deletions(-)
>> 
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index 3d07ca6..74446c6 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -646,10 +646,10 @@ ETEXI
>> 
>> {
>> .name   = "trace-events",
>> -.args_type  = "name:s?",
>> -.params = "[name]",
>> +.args_type  = "name:s?,vcpu:i?",
>> +.params = "[name] [vcpu]",
>> .help   = "show available trace-events & their state "
>> -  "(name: event name pattern)",
>> +  "(name: event name pattern; vcpu: vCPU to query, 
>> default is any)",
>> .mhandler.cmd = hmp_info_trace_events,
>> .command_completion = info_trace_events_completion,
>> },
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 98b4b1a..848efee 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -281,9 +281,10 @@ ETEXI
>> 
>> {
>> .name   = "trace-event",
>> -.args_type  = "name:s,option:b",
>> -.params = "name on|off",
>> -.help   = "changes status of a specific trace event",
>> +.args_type  = "name:s,option:b,vcpu:i?",
>> +.params = "name on|off [vcpu]",
>> +.help   = "changes status of a specific trace event "
>> +  "(vcpu: vCPU to set, default is all)",
>> .mhandler.cmd = hmp_trace_event,
>> .command_completion = trace_event_completion,
>> },
>> diff --git a/monitor.c b/monitor.c
>> index 7bd0f32..91a377b 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -908,9 +908,16 @@ static void hmp_trace_event(Monitor *mon, const QDict 
>> *qdict)
>> {
>> const char *tp_name = qdict_get_str(qdict, "name");
>> bool new_state = qdict_get_bool(qdict, "option");
>> +int vcpu = qdict_get_try_int(qdict, "vcpu", -1);
>> Error *local_err = NULL;
>> 
>> -qmp_trace_event_set_state(tp_name, new_state, true, true, _err);
>> +if (vcpu < -1) {
>> +/* some user-provided negative number */
>> +monitor_printf(mon, "argument vcpu must be positive");
>> +return;

> If -2 is not okay, why is -1 okay?

> What about:

> bool has_vcpu = qdict_haskey(qdict, "vcpu");
> int vcpu = qdict_get_try_int(qdict, "vcpu", 0);

> if (has_vcpu && vcpu < 0) {
[...]

I was trying to differentiate between the default value (-1) and other
user-provided negative values. I'll change it to use qdict_haskey() (I wasn't
aware of it).

Thanks,
  Lluis

Re: [Qemu-devel] [PATCH v7 7/7] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state

2016-06-30 Thread Markus Armbruster 
Lluís Vilanova  writes:

> Signed-off-by: Lluís Vilanova 
> Reviewed-by: Stefan Hajnoczi 
> ---
>  hmp-commands-info.hx |6 +-
>  hmp-commands.hx  |7 +-
>  monitor.c|   17 +-
>  qapi/trace.json  |   32 +--
>  qmp-commands.hx  |   35 +++-
>  trace/qmp.c  |  148 
> --
>  6 files changed, 202 insertions(+), 43 deletions(-)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 3d07ca6..74446c6 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -646,10 +646,10 @@ ETEXI
>  
>  {
>  .name   = "trace-events",
> -.args_type  = "name:s?",
> -.params = "[name]",
> +.args_type  = "name:s?,vcpu:i?",
> +.params = "[name] [vcpu]",
>  .help   = "show available trace-events & their state "
> -  "(name: event name pattern)",
> +  "(name: event name pattern; vcpu: vCPU to query, 
> default is any)",
>  .mhandler.cmd = hmp_info_trace_events,
>  .command_completion = info_trace_events_completion,
>  },
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 98b4b1a..848efee 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -281,9 +281,10 @@ ETEXI
>  
>  {
>  .name   = "trace-event",
> -.args_type  = "name:s,option:b",
> -.params = "name on|off",
> -.help   = "changes status of a specific trace event",
> +.args_type  = "name:s,option:b,vcpu:i?",
> +.params = "name on|off [vcpu]",
> +.help   = "changes status of a specific trace event "
> +  "(vcpu: vCPU to set, default is all)",
>  .mhandler.cmd = hmp_trace_event,
>  .command_completion = trace_event_completion,
>  },
> diff --git a/monitor.c b/monitor.c
> index 7bd0f32..91a377b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -908,9 +908,16 @@ static void hmp_trace_event(Monitor *mon, const QDict 
> *qdict)
>  {
>  const char *tp_name = qdict_get_str(qdict, "name");
>  bool new_state = qdict_get_bool(qdict, "option");
> +int vcpu = qdict_get_try_int(qdict, "vcpu", -1);
>  Error *local_err = NULL;
>  
> -qmp_trace_event_set_state(tp_name, new_state, true, true, _err);
> +if (vcpu < -1) {
> +/* some user-provided negative number */
> +monitor_printf(mon, "argument vcpu must be positive");
> +return;

If -2 is not okay, why is -1 okay?

What about:

bool has_vcpu = qdict_haskey(qdict, "vcpu");
int vcpu = qdict_get_try_int(qdict, "vcpu", 0);

if (has_vcpu && vcpu < 0) {

> +}
> +
> +qmp_trace_event_set_state(tp_name, new_state, true, true, vcpu != -1, 
> vcpu, _err);
>  if (local_err) {
>  error_report_err(local_err);
>  }
> @@ -1070,6 +1077,7 @@ static void hmp_info_cpustats(Monitor *mon, const QDict 
> *qdict)
>  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>  {
>  const char *name = qdict_get_try_str(qdict, "name");
> +int vcpu = qdict_get_try_int(qdict, "vcpu", -1);
>  TraceEventInfoList *events;
>  TraceEventInfoList *elem;
>  Error *local_err = NULL;
> @@ -1077,8 +1085,13 @@ static void hmp_info_trace_events(Monitor *mon, const 
> QDict *qdict)
>  if (name == NULL) {
>  name = "*";
>  }
> +if (vcpu < -1) {
> +/* some user-provided negative number */
> +monitor_printf(mon, "argument vcpu must be positive");
> +return;

Likewise.

> +}
>  
> -events = qmp_trace_event_get_state(name, _err);
> +events = qmp_trace_event_get_state(name, vcpu != -1, vcpu, _err);
>  if (local_err) {
>  error_report_err(local_err);
>  return;
[...]