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 <address@hidden>
>>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>>> ---
>>>> 
>>>>> 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