Re: [Qemu-devel] [PATCH] trace: Replace fprintf with error_report and print location

Re: [Qemu-devel] [PATCH] trace: Replace fprintf with error_report and print location

[Alexey Kardashevskiy]

On 05/31/2014 12:08 AM, Lluís Vilanova wrote:
> Alexey Kardashevskiy writes:
> 
>> On 05/29/2014 10:42 PM, Lluís Vilanova wrote:
>>> Alexey Kardashevskiy writes:
>>>
>>>> This replaces fprintf(stderr) with error_report.
>>>> This prints line number of the trace which does not exist or is not
>>>> traceable.
>>>
>>> A little nit pick; it shows an error when some of the events in the list of
>>> events to enable (not the trace) does not exist or is not traceable.
> 
> 
>> Sorry, I am not following you. It shows me this:
> 
>> qemu-system-ppc64:/home/aik/qemu_trace_events:75: WARNING: trace event
>> 'mememe' does not exist
> 
>> and continues. This exactly what I wanted.
> 
> Yes, that's right. All I'm saying is that the commit message is a little bit
> misleading. These are not the line numbers of a trace, but the line numbers of a
> list of events to enable when QEMU starts.


Like this? Thanks.

===
This replaces fprintf(stderr) with error_report.

This prints the line number of a list of events which does not exist or is
not traceable.

This moves local variables to the beginning of the function because of
the QEMU coding style.
===




> 
> 
> Thanks,
>   Lluis
> 
> 
>>>
>>>
>>> Thanks,
>>> Lluis
>>>
>>>
>>>> This moves local variables to the beginning of the function because of
>>>> the QEMU coding style.
>>>
>>>> Suggested-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>
>>>> Lluís, or it is s/Suggested-by/From/ ?
>>>
>>>> Stefan, this is made on top of "trace: Replace error with warning if event is not defined"
>>>
>>>> ---
>>>> trace/control.c | 31 ++++++++++++++++++-------------
>>>> 1 file changed, 18 insertions(+), 13 deletions(-)
>>>
>>>> diff --git a/trace/control.c b/trace/control.c
>>>> index 4aa02cf..4ee2bd2 100644
>>>> --- a/trace/control.c
>>>> +++ b/trace/control.c
>>>> @@ -8,7 +8,7 @@
>>>> */
>>>
>>>> #include "trace/control.h"
>>>> -
>>>> +#include "qemu/error-report.h"
>>>
>>>> TraceEvent *trace_event_name(const char *name)
>>>> {
>>>> @@ -81,18 +81,24 @@ TraceEvent *trace_event_pattern(const char *pat, TraceEvent *ev)
>>>
>>>> void trace_backend_init_events(const char *fname)
>>>> {
>>>> +    Location loc;
>>>> +    FILE *fp;
>>>> +    char line_buf[1024];
>>>> +    size_t line_idx = 0;
>>>> +
>>>> if (fname == NULL) {
>>>> return;
>>>> }
>>>
>>>> -    FILE *fp = fopen(fname, "r");
>>>> +    loc_push_none(&loc);
>>>> +    loc_set_file(fname, 0);
>>>> +    fp = fopen(fname, "r");
>>>> if (!fp) {
>>>> -        fprintf(stderr, "error: could not open trace events file '%s': %s\n",
>>>> -                fname, strerror(errno));
>>>> +        error_report("%s", strerror(errno));
>>>> exit(1);
>>>> }
>>>> -    char line_buf[1024];
>>>> while (fgets(line_buf, sizeof(line_buf), fp)) {
>>>> +        loc_set_file(fname, ++line_idx);
>>>> size_t len = strlen(line_buf);
>>>> if (len > 1) {              /* skip empty lines */
>>>> line_buf[len - 1] = '\0';
>>>> @@ -111,13 +117,11 @@ void trace_backend_init_events(const char *fname)
>>>> } else {
>>>> TraceEvent *ev = trace_event_name(line_ptr);
>>>> if (ev == NULL) {
>>>> -                    fprintf(stderr,
>>>> -                            "WARNING: trace event '%s' does not exist\n",
>>>> -                            line_ptr);
>>>> +                    error_report("WARNING: trace event '%s' does not exist",
>>>> +                                 line_ptr);
>>>> } else if (!trace_event_get_state_static(ev)) {
>>>> -                    fprintf(stderr,
>>>> -                            "WARNING: trace event '%s' is not traceable\n",
>>>> -                            line_ptr);
>>>> +                    error_report("WARNING: trace event '%s' is not traceable\n",
>>>> +                                 line_ptr);
>>>> } else {
>>>> trace_event_set_state_dynamic(ev, enable);
>>>> }
>>>> @@ -125,8 +129,9 @@ void trace_backend_init_events(const char *fname)
>>>> }
>>>> }
>>>> if (fclose(fp) != 0) {
>>>> -        fprintf(stderr, "error: closing file '%s': %s\n",
>>>> -                fname, strerror(errno));
>>>> +        loc_set_file(fname, 0);
>>>> +        error_report("%s", strerror(errno));
>>>> exit(1);
>>>> }
>>>> +    loc_pop(&loc);
>>>> }





-- 
Alexey

Re: [Qemu-devel] [PATCH] trace: Replace fprintf with error_report and print location

[Lluís Vilanova]

Alexey Kardashevskiy writes:

> On 05/31/2014 12:08 AM, Lluís Vilanova wrote:
>> Alexey Kardashevskiy writes:
>> 
>>> On 05/29/2014 10:42 PM, Lluís Vilanova wrote:
>>>> Alexey Kardashevskiy writes:
>>>> 
>>>>> This replaces fprintf(stderr) with error_report.
>>>>> This prints line number of the trace which does not exist or is not
>>>>> traceable.
>>>> 
>>>> A little nit pick; it shows an error when some of the events in the list of
>>>> events to enable (not the trace) does not exist or is not traceable.
>> 
>> 
>>> Sorry, I am not following you. It shows me this:
>> 
>>> qemu-system-ppc64:/home/aik/qemu_trace_events:75: WARNING: trace event
>>> 'mememe' does not exist
>> 
>>> and continues. This exactly what I wanted.
>> 
>> Yes, that's right. All I'm saying is that the commit message is a little bit
>> misleading. These are not the line numbers of a trace, but the line numbers of a
>> list of events to enable when QEMU starts.


> Like this? Thanks.

> ===
> This replaces fprintf(stderr) with error_report.

> This prints the line number of a list of events which does not exist or is
> not traceable.

> This moves local variables to the beginning of the function because of
> the QEMU coding style.
> ===

I think there is no need to describe what that function is doing. Instead I
would say something like:

  Replaces fprintf(stdeer) with error_report.

  Also moves some local variables to the beginning of the function to comply
  with QEMU's coding style.


Thanks,
  Lluis



>> 
>> 
>> Thanks,
>> Lluis
>> 
>> 
>>>> 
>>>> 
>>>> Thanks,
>>>> Lluis
>>>> 
>>>> 
>>>>> This moves local variables to the beginning of the function because of
>>>>> the QEMU coding style.
>>>> 
>>>>> Suggested-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>> 
>>>>> Lluís, or it is s/Suggested-by/From/ ?
>>>> 
>>>>> Stefan, this is made on top of "trace: Replace error with warning if event is not defined"
>>>> 
>>>>> ---
>>>>> trace/control.c | 31 ++++++++++++++++++-------------
>>>>> 1 file changed, 18 insertions(+), 13 deletions(-)
>>>> 
>>>>> diff --git a/trace/control.c b/trace/control.c
>>>>> index 4aa02cf..4ee2bd2 100644
>>>>> --- a/trace/control.c
>>>>> +++ b/trace/control.c
>>>>> @@ -8,7 +8,7 @@
>>>>> */
>>>> 
>>>>> #include "trace/control.h"
>>>>> -
>>>>> +#include "qemu/error-report.h"
>>>> 
>>>>> TraceEvent *trace_event_name(const char *name)
>>>>> {
>>>>> @@ -81,18 +81,24 @@ TraceEvent *trace_event_pattern(const char *pat, TraceEvent *ev)
>>>> 
>>>>> void trace_backend_init_events(const char *fname)
>>>>> {
>>>>> +    Location loc;
>>>>> +    FILE *fp;
>>>>> +    char line_buf[1024];
>>>>> +    size_t line_idx = 0;
>>>>> +
>>>>> if (fname == NULL) {
>>>>> return;
>>>>> }
>>>> 
>>>>> -    FILE *fp = fopen(fname, "r");
>>>>> +    loc_push_none(&loc);
>>>>> +    loc_set_file(fname, 0);
>>>>> +    fp = fopen(fname, "r");
>>>>> if (!fp) {
>>>>> -        fprintf(stderr, "error: could not open trace events file '%s': %s\n",
>>>>> -                fname, strerror(errno));
>>>>> +        error_report("%s", strerror(errno));
>>>>> exit(1);
>>>>> }
>>>>> -    char line_buf[1024];
>>>>> while (fgets(line_buf, sizeof(line_buf), fp)) {
>>>>> +        loc_set_file(fname, ++line_idx);
>>>>> size_t len = strlen(line_buf);
>>>>> if (len > 1) {              /* skip empty lines */
>>>>> line_buf[len - 1] = '\0';
>>>>> @@ -111,13 +117,11 @@ void trace_backend_init_events(const char *fname)
>>>>> } else {
>>>>> TraceEvent *ev = trace_event_name(line_ptr);
>>>>> if (ev == NULL) {
>>>>> -                    fprintf(stderr,
>>>>> -                            "WARNING: trace event '%s' does not exist\n",
>>>>> -                            line_ptr);
>>>>> +                    error_report("WARNING: trace event '%s' does not exist",
>>>>> +                                 line_ptr);
>>>>> } else if (!trace_event_get_state_static(ev)) {
>>>>> -                    fprintf(stderr,
>>>>> -                            "WARNING: trace event '%s' is not traceable\n",
>>>>> -                            line_ptr);
>>>>> +                    error_report("WARNING: trace event '%s' is not traceable\n",
>>>>> +                                 line_ptr);
>>>>> } else {
>>>>> trace_event_set_state_dynamic(ev, enable);
>>>>> }
>>>>> @@ -125,8 +129,9 @@ void trace_backend_init_events(const char *fname)
>>>>> }
>>>>> }
>>>>> if (fclose(fp) != 0) {
>>>>> -        fprintf(stderr, "error: closing file '%s': %s\n",
>>>>> -                fname, strerror(errno));
>>>>> +        loc_set_file(fname, 0);
>>>>> +        error_report("%s", strerror(errno));
>>>>> exit(1);
>>>>> }
>>>>> +    loc_pop(&loc);
>>>>> }





> -- 
> Alexey

-- 
 "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