| Message ID | 1454514465-11856-6-git-send-email-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Stefan Hajnoczi <stefanha@redhat.com> writes: > From: Paolo Bonzini <pbonzini@redhat.com> > > This is cleaner, and improves error reporting with -daemonize. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > Message-id: 1452174932-28657-4-git-send-email-den@openvz.org > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > qemu-io.c | 2 +- > trace/control.c | 17 ++++++++++++----- > trace/control.h | 13 ++++++++++++- > trace/simple.c | 6 ++---- > trace/simple.h | 4 ++-- > vl.c | 13 +++++++++---- > 6 files changed, 38 insertions(+), 17 deletions(-) > > diff --git a/qemu-io.c b/qemu-io.c > index 1c11d57..83c48f4 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -435,7 +435,7 @@ int main(int argc, char **argv) > } > break; > case 'T': > - if (!trace_init_backends(optarg)) { > + if (!trace_init_backends()) { > exit(1); /* error message will have been printed */ > } > break; > diff --git a/trace/control.c b/trace/control.c > index 931d64c..f5a497a 100644 > --- a/trace/control.c > +++ b/trace/control.c > @@ -145,17 +145,24 @@ void trace_init_events(const char *fname) > loc_pop(&loc); > } > > -bool trace_init_backends(const char *file) > +void trace_init_file(const char *file) > { > #ifdef CONFIG_TRACE_SIMPLE > - if (!st_init(file)) { > - fprintf(stderr, "failed to initialize simple tracing backend.\n"); > - return false; > - } > + st_set_trace_file(file); This breaks "make check" as st_set_trace_file will attempt to flush the file: /* Halt trace writeout */ flush_trace_file(true); trace_writeout_enabled = false; flush_trace_file(true); And this deadlocks waiting for trace_empty_cond to get tickled which will never happen because: <snip> > diff --git a/vl.c b/vl.c > index ff2a7d5..955f364 100644 > --- a/vl.c > +++ b/vl.c > @@ -2988,7 +2988,7 @@ int main(int argc, char **argv, char **envp) > bool userconfig = true; > const char *log_mask = NULL; > const char *log_file = NULL; > - const char *trace_file = NULL; > + char *trace_file = NULL; > ram_addr_t maxram_size; > uint64_t ram_slots = 0; > FILE *vmstate_dump_file = NULL; > @@ -3905,7 +3905,10 @@ int main(int argc, char **argv, char **envp) > exit(1); > } > trace_init_events(qemu_opt_get(opts, "events")); > - trace_file = qemu_opt_get(opts, "file"); > + if (trace_file) { > + g_free(trace_file); > + } > + trace_file = g_strdup(qemu_opt_get(opts, "file")); > qemu_opts_del(opts); > break; > } > @@ -4089,6 +4092,8 @@ int main(int argc, char **argv, char **envp) > exit(0); > } > > + trace_init_file(trace_file); > + This happens before: > */ > if (log_file) { > @@ -4106,7 +4111,7 @@ int main(int argc, char **argv, char **envp) > } > > if (!is_daemonized()) { > - if (!trace_init_backends(trace_file)) { > + if (!trace_init_backends()) { > exit(1); > } > } > @@ -4653,7 +4658,7 @@ int main(int argc, char **argv, char **envp) > os_setup_post(); > > if (is_daemonized()) { > - if (!trace_init_backends(trace_file)) { > + if (!trace_init_backends()) { This which creates the thread. > exit(1); > } > } -- Alex Bennée
On 02/08/2016 09:43 PM, Alex Bennée wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > >> From: Paolo Bonzini <pbonzini@redhat.com> >> >> This is cleaner, and improves error reporting with -daemonize. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Message-id: 1452174932-28657-4-git-send-email-den@openvz.org >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >> --- >> qemu-io.c | 2 +- >> trace/control.c | 17 ++++++++++++----- >> trace/control.h | 13 ++++++++++++- >> trace/simple.c | 6 ++---- >> trace/simple.h | 4 ++-- >> vl.c | 13 +++++++++---- >> 6 files changed, 38 insertions(+), 17 deletions(-) >> >> diff --git a/qemu-io.c b/qemu-io.c >> index 1c11d57..83c48f4 100644 >> --- a/qemu-io.c >> +++ b/qemu-io.c >> @@ -435,7 +435,7 @@ int main(int argc, char **argv) >> } >> break; >> case 'T': >> - if (!trace_init_backends(optarg)) { >> + if (!trace_init_backends()) { >> exit(1); /* error message will have been printed */ >> } >> break; >> diff --git a/trace/control.c b/trace/control.c >> index 931d64c..f5a497a 100644 >> --- a/trace/control.c >> +++ b/trace/control.c >> @@ -145,17 +145,24 @@ void trace_init_events(const char *fname) >> loc_pop(&loc); >> } >> >> -bool trace_init_backends(const char *file) >> +void trace_init_file(const char *file) >> { >> #ifdef CONFIG_TRACE_SIMPLE >> - if (!st_init(file)) { >> - fprintf(stderr, "failed to initialize simple tracing backend.\n"); >> - return false; >> - } >> + st_set_trace_file(file); > This breaks "make check" as st_set_trace_file will attempt to flush the > file: this does not hang for me even with CONFIG_TRACE_SIMPLE enabled. Could you share your ./configure options? Den
Denis V. Lunev <den@openvz.org> writes: > On 02/08/2016 09:43 PM, Alex Bennée wrote: >> Stefan Hajnoczi <stefanha@redhat.com> writes: >> >>> From: Paolo Bonzini <pbonzini@redhat.com> >>> >>> This is cleaner, and improves error reporting with -daemonize. >>> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> Message-id: 1452174932-28657-4-git-send-email-den@openvz.org >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> qemu-io.c | 2 +- >>> trace/control.c | 17 ++++++++++++----- >>> trace/control.h | 13 ++++++++++++- >>> trace/simple.c | 6 ++---- >>> trace/simple.h | 4 ++-- >>> vl.c | 13 +++++++++---- >>> 6 files changed, 38 insertions(+), 17 deletions(-) >>> >>> diff --git a/qemu-io.c b/qemu-io.c >>> index 1c11d57..83c48f4 100644 >>> --- a/qemu-io.c >>> +++ b/qemu-io.c >>> @@ -435,7 +435,7 @@ int main(int argc, char **argv) >>> } >>> break; >>> case 'T': >>> - if (!trace_init_backends(optarg)) { >>> + if (!trace_init_backends()) { >>> exit(1); /* error message will have been printed */ >>> } >>> break; >>> diff --git a/trace/control.c b/trace/control.c >>> index 931d64c..f5a497a 100644 >>> --- a/trace/control.c >>> +++ b/trace/control.c >>> @@ -145,17 +145,24 @@ void trace_init_events(const char *fname) >>> loc_pop(&loc); >>> } >>> >>> -bool trace_init_backends(const char *file) >>> +void trace_init_file(const char *file) >>> { >>> #ifdef CONFIG_TRACE_SIMPLE >>> - if (!st_init(file)) { >>> - fprintf(stderr, "failed to initialize simple tracing backend.\n"); >>> - return false; >>> - } >>> + st_set_trace_file(file); >> This breaks "make check" as st_set_trace_file will attempt to flush the >> file: > this does not hang for me even with CONFIG_TRACE_SIMPLE enabled. > Could you share your ./configure options? ./configure --target-list=x86_64-softmmu --enable-trace-backends=simple Also see: https://travis-ci.org/stsquad/qemu/jobs/107280320 -- Alex Bennée
On 02/09/2016 03:28 PM, Alex Bennée wrote: > Denis V. Lunev <den@openvz.org> writes: > >> On 02/08/2016 09:43 PM, Alex Bennée wrote: >>> Stefan Hajnoczi <stefanha@redhat.com> writes: >>> >>>> From: Paolo Bonzini <pbonzini@redhat.com> >>>> >>>> This is cleaner, and improves error reporting with -daemonize. >>>> >>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> Message-id: 1452174932-28657-4-git-send-email-den@openvz.org >>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>>> --- >>>> qemu-io.c | 2 +- >>>> trace/control.c | 17 ++++++++++++----- >>>> trace/control.h | 13 ++++++++++++- >>>> trace/simple.c | 6 ++---- >>>> trace/simple.h | 4 ++-- >>>> vl.c | 13 +++++++++---- >>>> 6 files changed, 38 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/qemu-io.c b/qemu-io.c >>>> index 1c11d57..83c48f4 100644 >>>> --- a/qemu-io.c >>>> +++ b/qemu-io.c >>>> @@ -435,7 +435,7 @@ int main(int argc, char **argv) >>>> } >>>> break; >>>> case 'T': >>>> - if (!trace_init_backends(optarg)) { >>>> + if (!trace_init_backends()) { >>>> exit(1); /* error message will have been printed */ >>>> } >>>> break; >>>> diff --git a/trace/control.c b/trace/control.c >>>> index 931d64c..f5a497a 100644 >>>> --- a/trace/control.c >>>> +++ b/trace/control.c >>>> @@ -145,17 +145,24 @@ void trace_init_events(const char *fname) >>>> loc_pop(&loc); >>>> } >>>> >>>> -bool trace_init_backends(const char *file) >>>> +void trace_init_file(const char *file) >>>> { >>>> #ifdef CONFIG_TRACE_SIMPLE >>>> - if (!st_init(file)) { >>>> - fprintf(stderr, "failed to initialize simple tracing backend.\n"); >>>> - return false; >>>> - } >>>> + st_set_trace_file(file); >>> This breaks "make check" as st_set_trace_file will attempt to flush the >>> file: >> this does not hang for me even with CONFIG_TRACE_SIMPLE enabled. >> Could you share your ./configure options? > ./configure --target-list=x86_64-softmmu --enable-trace-backends=simple > > Also see: > > https://travis-ci.org/stsquad/qemu/jobs/107280320 > > -- > Alex Bennée for the reference (proposed fix): http://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg02123.html
diff --git a/qemu-io.c b/qemu-io.c index 1c11d57..83c48f4 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -435,7 +435,7 @@ int main(int argc, char **argv) } break; case 'T': - if (!trace_init_backends(optarg)) { + if (!trace_init_backends()) { exit(1); /* error message will have been printed */ } break; diff --git a/trace/control.c b/trace/control.c index 931d64c..f5a497a 100644 --- a/trace/control.c +++ b/trace/control.c @@ -145,17 +145,24 @@ void trace_init_events(const char *fname) loc_pop(&loc); } -bool trace_init_backends(const char *file) +void trace_init_file(const char *file) { #ifdef CONFIG_TRACE_SIMPLE - if (!st_init(file)) { - fprintf(stderr, "failed to initialize simple tracing backend.\n"); - return false; - } + st_set_trace_file(file); #else if (file) { fprintf(stderr, "error: -trace file=...: " "option not supported by the selected tracing backends\n"); + exit(1); + } +#endif +} + +bool trace_init_backends(void) +{ +#ifdef CONFIG_TRACE_SIMPLE + if (!st_init()) { + fprintf(stderr, "failed to initialize simple tracing backend.\n"); return false; } #endif diff --git a/trace/control.h b/trace/control.h index 7905917..d50f399 100644 --- a/trace/control.h +++ b/trace/control.h @@ -157,7 +157,7 @@ static void trace_event_set_state_dynamic(TraceEvent *ev, bool state); * * Returns: Whether the backends could be successfully initialized. */ -bool trace_init_backends(const char *file); +bool trace_init_backends(void); /** * trace_init_events: @@ -170,6 +170,17 @@ bool trace_init_backends(const char *file); */ void trace_init_events(const char *file); +/** + * trace_init_file: + * @file: Name of trace output file; may be NULL. + * Corresponds to commandline option "-trace file=...". + * + * Record the name of the output file for the tracing backend. + * Exits if no selected backend does not support specifying the + * output file, and a non-NULL file was passed. + */ +void trace_init_file(const char *file); + #include "trace/control-internal.h" diff --git a/trace/simple.c b/trace/simple.c index 56a624c..e8594cd 100644 --- a/trace/simple.c +++ b/trace/simple.c @@ -322,7 +322,7 @@ void st_set_trace_file_enabled(bool enable) * @file The trace file name or NULL for the default name-<pid> set at * config time */ -bool st_set_trace_file(const char *file) +void st_set_trace_file(const char *file) { st_set_trace_file_enabled(false); @@ -336,7 +336,6 @@ bool st_set_trace_file(const char *file) } st_set_trace_file_enabled(true); - return true; } void st_print_trace_file_status(FILE *stream, int (*stream_printf)(FILE *stream, const char *fmt, ...)) @@ -374,7 +373,7 @@ static GThread *trace_thread_create(GThreadFunc fn) return thread; } -bool st_init(const char *file) +bool st_init(void) { GThread *thread; @@ -387,6 +386,5 @@ bool st_init(const char *file) } atexit(st_flush_trace_buffer); - st_set_trace_file(file); return true; } diff --git a/trace/simple.h b/trace/simple.h index 6997996..8d1a32e 100644 --- a/trace/simple.h +++ b/trace/simple.h @@ -20,8 +20,8 @@ void st_print_trace_file_status(FILE *stream, fprintf_function stream_printf); void st_set_trace_file_enabled(bool enable); -bool st_set_trace_file(const char *file); -bool st_init(const char *file); +void st_set_trace_file(const char *file); +bool st_init(void); void st_flush_trace_buffer(void); typedef struct { diff --git a/vl.c b/vl.c index ff2a7d5..955f364 100644 --- a/vl.c +++ b/vl.c @@ -2988,7 +2988,7 @@ int main(int argc, char **argv, char **envp) bool userconfig = true; const char *log_mask = NULL; const char *log_file = NULL; - const char *trace_file = NULL; + char *trace_file = NULL; ram_addr_t maxram_size; uint64_t ram_slots = 0; FILE *vmstate_dump_file = NULL; @@ -3905,7 +3905,10 @@ int main(int argc, char **argv, char **envp) exit(1); } trace_init_events(qemu_opt_get(opts, "events")); - trace_file = qemu_opt_get(opts, "file"); + if (trace_file) { + g_free(trace_file); + } + trace_file = g_strdup(qemu_opt_get(opts, "file")); qemu_opts_del(opts); break; } @@ -4089,6 +4092,8 @@ int main(int argc, char **argv, char **envp) exit(0); } + trace_init_file(trace_file); + /* Open the logfile at this point and set the log mask if necessary. */ if (log_file) { @@ -4106,7 +4111,7 @@ int main(int argc, char **argv, char **envp) } if (!is_daemonized()) { - if (!trace_init_backends(trace_file)) { + if (!trace_init_backends()) { exit(1); } } @@ -4653,7 +4658,7 @@ int main(int argc, char **argv, char **envp) os_setup_post(); if (is_daemonized()) { - if (!trace_init_backends(trace_file)) { + if (!trace_init_backends()) { exit(1); } }