diff mbox

[PULL,05/13] trace: split trace_init_file out of trace_init_backends

Message ID 1454514465-11856-6-git-send-email-stefanha@redhat.com
State New
Headers show

Commit Message

Stefan Hajnoczi Feb. 3, 2016, 3:47 p.m. UTC 
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(-)

Comments

Alex Bennée Feb. 8, 2016, 6:43 p.m. UTC | #1 
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
Denis V. Lunev Feb. 9, 2016, 11:17 a.m. UTC | #2 
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
Alex Bennée Feb. 9, 2016, 12:28 p.m. UTC | #3 
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
Denis V. Lunev Feb. 9, 2016, 7:19 p.m. UTC | #4 
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 mbox

Patch

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);
         }
     }