| Message ID | 1401332129-15677-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
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. 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); > } > -- > 1.8.4.rc4
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. > > > 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); >> } >> -- >> 1.8.4.rc4 > >
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. 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); >>> } >>> -- >>> 1.8.4.rc4 >> >> > -- > Alexey
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 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
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); }
This replaces fprintf(stderr) with error_report. This prints line number of the trace which does not exist or is not traceable. 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(-)