[Qemu-devel] [PULL 0/6 1.2] Tracing patches for QEMU 1.2

[Qemu-devel] [PULL 0/6 1.2] Tracing patches for QEMU 1.2

[Stefan Hajnoczi]

The last pull request from 19 July was not merged.  Here it is rebased on
qemu.git/master with two additional fixes from Stefan Weil.

The following changes since commit 633decd71119a4293e5e53e6059026c517a8bef0:

  Merge remote-tracking branch 'qmp/queue/qmp' into staging (2012-08-13 16:12:35 -0500)

are available in the git repository at:


  git://github.com/stefanha/qemu.git tracing

for you to fetch changes up to 4552e41025af4694c55854448c3ae4d95e72c7f6:

  trace/simple: Replace asprintf by g_strdup_printf (2012-08-14 13:19:57 +0100)

----------------------------------------------------------------
Harsh Prateek Bora (4):
      trace: rename TraceRecordHeader to TraceLogHeader
      trace: remove unnecessary write_to_buffer() typecasting
      trace: drop unused TraceBufferRecord->next_tbuf_idx field
      trace: avoid pointer aliasing in trace_record_finish()

Stefan Weil (2):
      trace/simple: Fix compiler warning for 32 bit hosts
      trace/simple: Replace asprintf by g_strdup_printf

 scripts/tracetool/backend/simple.py |    2 +-
 trace/simple.c                      |   35 +++++++++++++----------------------
 trace/simple.h                      |    1 -
 3 files changed, 14 insertions(+), 24 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/6] trace: rename TraceRecordHeader to TraceLogHeader

[Stefan Hajnoczi]

From: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>

The TraceRecordHeader is really the header for the entire trace log
file.  It's not per-record header so make this obvious by renaming it.

Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 trace/simple.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/trace/simple.c b/trace/simple.c
index b700ea3..5d92939 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -70,7 +70,7 @@ typedef struct {
     uint64_t header_event_id; /* HEADER_EVENT_ID */
     uint64_t header_magic;    /* HEADER_MAGIC    */
     uint64_t header_version;  /* HEADER_VERSION  */
-} TraceRecordHeader;
+} TraceLogHeader;
 
 
 static void read_from_buffer(unsigned int idx, void *dataptr, size_t size);
@@ -295,7 +295,7 @@ void st_set_trace_file_enabled(bool enable)
     flush_trace_file(true);
 
     if (enable) {
-        static const TraceRecordHeader header = {
+        static const TraceLogHeader header = {
             .header_event_id = HEADER_EVENT_ID,
             .header_magic = HEADER_MAGIC,
             /* Older log readers will check for version at next location */
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/6] trace: remove unnecessary write_to_buffer() typecasting

[Stefan Hajnoczi]

From: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>

The buffer argument is void* so it is not necessary to cast.

Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 trace/simple.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/trace/simple.c b/trace/simple.c
index 5d92939..a0e0f05 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -235,9 +235,9 @@ int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasi
     rec->next_tbuf_idx = new_idx % TRACE_BUF_LEN;
 
     rec_off = idx;
-    rec_off = write_to_buffer(rec_off, (uint8_t*)&event, sizeof(event));
-    rec_off = write_to_buffer(rec_off, (uint8_t*)&timestamp_ns, sizeof(timestamp_ns));
-    rec_off = write_to_buffer(rec_off, (uint8_t*)&rec_len, sizeof(rec_len));
+    rec_off = write_to_buffer(rec_off, &event, sizeof(event));
+    rec_off = write_to_buffer(rec_off, &timestamp_ns, sizeof(timestamp_ns));
+    rec_off = write_to_buffer(rec_off, &rec_len, sizeof(rec_len));
 
     rec->tbuf_idx = idx;
     rec->rec_off  = (idx + sizeof(TraceRecord)) % TRACE_BUF_LEN;
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/6] trace: drop unused TraceBufferRecord->next_tbuf_idx field

[Stefan Hajnoczi]

From: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>

Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 trace/simple.c |    2 --
 trace/simple.h |    1 -
 2 files changed, 3 deletions(-)

diff --git a/trace/simple.c b/trace/simple.c
index a0e0f05..4fed07f 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -231,8 +231,6 @@ int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasi
     }
 
     idx = old_idx % TRACE_BUF_LEN;
-    /*  To check later if threshold crossed */
-    rec->next_tbuf_idx = new_idx % TRACE_BUF_LEN;
 
     rec_off = idx;
     rec_off = write_to_buffer(rec_off, &event, sizeof(event));
diff --git a/trace/simple.h b/trace/simple.h
index 7e521c1..2ab96a8 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -29,7 +29,6 @@ void st_flush_trace_buffer(void);
 
 typedef struct {
     unsigned int tbuf_idx;
-    unsigned int next_tbuf_idx;
     unsigned int rec_off;
 } TraceBufferRecord;
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/6] trace: avoid pointer aliasing in trace_record_finish()

[Stefan Hajnoczi]

From: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>

Declaring a TraceRecord on the stack works fine.  No need for a
uint8_t array and pointer aliasing.

Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 trace/simple.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/trace/simple.c b/trace/simple.c
index 4fed07f..8e175ec 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -269,12 +269,11 @@ static unsigned int write_to_buffer(unsigned int idx, void *dataptr, size_t size
 
 void trace_record_finish(TraceBufferRecord *rec)
 {
-    uint8_t temp_rec[sizeof(TraceRecord)];
-    TraceRecord *record = (TraceRecord *) temp_rec;
-    read_from_buffer(rec->tbuf_idx, temp_rec, sizeof(TraceRecord));
+    TraceRecord record;
+    read_from_buffer(rec->tbuf_idx, &record, sizeof(TraceRecord));
     smp_wmb(); /* write barrier before marking as valid */
-    record->event |= TRACE_RECORD_VALID;
-    write_to_buffer(rec->tbuf_idx, temp_rec, sizeof(TraceRecord));
+    record.event |= TRACE_RECORD_VALID;
+    write_to_buffer(rec->tbuf_idx, &record, sizeof(TraceRecord));
 
     if ((trace_idx - writeout_idx) > TRACE_BUF_FLUSH_THRESHOLD) {
         flush_trace_file(false);
-- 
1.7.10.4

[Qemu-devel] [PATCH 5/6] trace/simple: Fix compiler warning for 32 bit hosts

[Stefan Hajnoczi]

From: Stefan Weil <sw@weilnetz.de>

gcc complains when a 32 bit pointer is casted to a 64 bit integer.

Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 scripts/tracetool/backend/simple.py |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
index c7e47d6..e4b4a7f 100644
--- a/scripts/tracetool/backend/simple.py
+++ b/scripts/tracetool/backend/simple.py
@@ -79,7 +79,7 @@ def c(events):
                        )
                 # pointer var (not string)
                 elif type_.endswith('*'):
-                    out('    trace_record_write_u64(&rec, (uint64_t)(uint64_t *)%(name)s);',
+                    out('    trace_record_write_u64(&rec, (uintptr_t)(uint64_t *)%(name)s);',
                         name = name,
                        )
                 # primitive data type
-- 
1.7.10.4

[Qemu-devel] [PATCH 6/6] trace/simple: Replace asprintf by g_strdup_printf

[Stefan Hajnoczi]

From: Stefan Weil <sw@weilnetz.de>

asprintf is not available for all hosts. g_strdup_printf is
more portable and simplifies the code because if does not
need error handling.

The static variable does not need an explicit assignment to be NULL.

Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 trace/simple.c |   14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/trace/simple.c b/trace/simple.c
index 8e175ec..d83681b 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -55,7 +55,7 @@ static unsigned int trace_idx;
 static unsigned int writeout_idx;
 static uint64_t dropped_events;
 static FILE *trace_fp;
-static char *trace_file_name = NULL;
+static char *trace_file_name;
 
 /* * Trace buffer entry */
 typedef struct {
@@ -329,18 +329,12 @@ bool st_set_trace_file(const char *file)
 {
     st_set_trace_file_enabled(false);
 
-    free(trace_file_name);
+    g_free(trace_file_name);
 
     if (!file) {
-        if (asprintf(&trace_file_name, CONFIG_TRACE_FILE, getpid()) < 0) {
-            trace_file_name = NULL;
-            return false;
-        }
+        trace_file_name = g_strdup_printf(CONFIG_TRACE_FILE, getpid());
     } else {
-        if (asprintf(&trace_file_name, "%s", file) < 0) {
-            trace_file_name = NULL;
-            return false;
-        }
+        trace_file_name = g_strdup_printf("%s", file);
     }
 
     st_set_trace_file_enabled(true);
-- 
1.7.10.4

Re: [Qemu-devel] [PULL 0/6 1.2] Tracing patches for QEMU 1.2

[Anthony Liguori]

Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> writes:

> The last pull request from 19 July was not merged.  Here it is rebased on
> qemu.git/master with two additional fixes from Stefan Weil.

I'm not sure what you mean:

commit 903f650b0c77827f8d92b35f61419401d648df1e
Merge: 61dc008 90a147a
Author: Anthony Liguori <aliguori@us.ibm.com>
Date:   Mon Jul 23 13:15:34 2012 -0500

    Merge remote-tracking branch 'stefanha/tracing' into staging
    
    * stefanha/tracing:
      Update simpletrace.py for new log format
      Simpletrace v2: Support multiple arguments, strings.
      monitor: remove unused do_info_trace
      trace: added ability to comment out events in the list


Your pull request was for 90a147a275da3a432bdf00238ebf438eff1d2c1b which
is what is being merged here.

What your pull request inaccurate?

Regards,

Anthony Liguori

>
> The following changes since commit 633decd71119a4293e5e53e6059026c517a8bef0:
>
>   Merge remote-tracking branch 'qmp/queue/qmp' into staging (2012-08-13 16:12:35 -0500)
>
> are available in the git repository at:
>
>
>   git://github.com/stefanha/qemu.git tracing
>
> for you to fetch changes up to 4552e41025af4694c55854448c3ae4d95e72c7f6:
>
>   trace/simple: Replace asprintf by g_strdup_printf (2012-08-14 13:19:57 +0100)
>
> ----------------------------------------------------------------
> Harsh Prateek Bora (4):
>       trace: rename TraceRecordHeader to TraceLogHeader
>       trace: remove unnecessary write_to_buffer() typecasting
>       trace: drop unused TraceBufferRecord->next_tbuf_idx field
>       trace: avoid pointer aliasing in trace_record_finish()
>
> Stefan Weil (2):
>       trace/simple: Fix compiler warning for 32 bit hosts
>       trace/simple: Replace asprintf by g_strdup_printf
>
>  scripts/tracetool/backend/simple.py |    2 +-
>  trace/simple.c                      |   35 +++++++++++++----------------------
>  trace/simple.h                      |    1 -
>  3 files changed, 14 insertions(+), 24 deletions(-)
>
> -- 
> 1.7.10.4

Re: [Qemu-devel] [PULL 0/6 1.2] Tracing patches for QEMU 1.2

[Stefan Hajnoczi]

On Tue, Aug 14, 2012 at 7:44 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> writes:
>
>> The last pull request from 19 July was not merged.  Here it is rebased on
>> qemu.git/master with two additional fixes from Stefan Weil.
>
> I'm not sure what you mean:
>
> commit 903f650b0c77827f8d92b35f61419401d648df1e
> Merge: 61dc008 90a147a
> Author: Anthony Liguori <aliguori@us.ibm.com>
> Date:   Mon Jul 23 13:15:34 2012 -0500
>
>     Merge remote-tracking branch 'stefanha/tracing' into staging
>
>     * stefanha/tracing:
>       Update simpletrace.py for new log format
>       Simpletrace v2: Support multiple arguments, strings.
>       monitor: remove unused do_info_trace
>       trace: added ability to comment out events in the list
>
>
> Your pull request was for 90a147a275da3a432bdf00238ebf438eff1d2c1b which
> is what is being merged here.
>
> What your pull request inaccurate?

You are right.  I was confused and thought I previously sent a pull
request with Harsh's cleanup patches when I didn't.

The pull request I raised yesterday
(4552e41025af4694c55854448c3ae4d95e72c7f6) is good though.

Sorry,
Stefan