[PATCH 0/4] Add dtrace support on macOS

[PATCH 0/4] Add dtrace support on macOS

[Roman Bolshakov]

Hi,

This is a small series that enables dtrace tracing backend on macOS.
Whether or not it should go to 5.1 is up to discretion of tracing
maintainers.

Thanks,
Roman

Roman Bolshakov (4):
  scripts/tracetool: Fix dtrace generation for macOS
  scripts/tracetool: Use void pointer for vcpu
  build: Don't make object files for dtrace on macOS
  net/colo: Match is-enabled probe to tracepoint

 Makefile.objs                 |  2 ++
 net/colo-compare.c            | 12 +++++++-----
 net/filter-rewriter.c         |  8 ++++++--
 net/trace-events              |  2 --
 scripts/tracetool/format/d.py |  9 ++++++++-
 scripts/tracetool/vcpu.py     |  2 +-
 6 files changed, 24 insertions(+), 11 deletions(-)

-- 
2.26.1

[PATCH 1/4] scripts/tracetool: Fix dtrace generation for macOS

[Roman Bolshakov]

dtrace USDT is fully supported since OS X 10.6. There are a few
peculiarities compared to other dtrace flavors.

1. It doesn't accept empty files.
2. It doesn't recognize bool type but accepts ANSI C _Bool.

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 scripts/tracetool/format/d.py | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/tracetool/format/d.py b/scripts/tracetool/format/d.py
index 0afb5f3f47..be4a2aa254 100644
--- a/scripts/tracetool/format/d.py
+++ b/scripts/tracetool/format/d.py
@@ -13,6 +13,7 @@ __email__      = "stefanha@redhat.com"
 
 
 from tracetool import out
+from sys import platform
 
 
 # Reserved keywords from
@@ -34,7 +35,8 @@ def generate(events, backend, group):
 
     # SystemTap's dtrace(1) warns about empty "provider qemu {}" but is happy
     # with an empty file.  Avoid the warning.
-    if not events:
+    # But dtrace on macOS can't deal with empty files.
+    if not events and platform != "darwin":
         return
 
     out('/* This file is autogenerated by tracetool, do not edit. */'
@@ -44,6 +46,11 @@ def generate(events, backend, group):
     for e in events:
         args = []
         for type_, name in e.args:
+            if platform == "darwin":
+                if type_ == 'bool':
+                    type_ = '_Bool'
+                if type_ == 'bool *':
+                    type_ = '_Bool *'
             if name in RESERVED_WORDS:
                 name += '_'
             args.append(type_ + ' ' + name)
-- 
2.26.1

[PATCH 2/4] scripts/tracetool: Use void pointer for vcpu

[Roman Bolshakov]

dtrace on macOS complains that CPUState * is used for a few probes:

  dtrace: failed to compile script trace-dtrace-root.dtrace: line 130: syntax error near "CPUState"

A comment in scripts/tracetool/__init__.py mentions that:

  We only want to allow standard C types or fixed sized
  integer types. We don't want QEMU specific types
  as we can't assume trace backends can resolve all the
  typedefs

Fixes: 3d211d9f4dbee ("trace: Add 'vcpu' event property to trace guest vCPU")
Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 scripts/tracetool/vcpu.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/tracetool/vcpu.py b/scripts/tracetool/vcpu.py
index b54e4f4e7a..868b4cb04c 100644
--- a/scripts/tracetool/vcpu.py
+++ b/scripts/tracetool/vcpu.py
@@ -24,7 +24,7 @@ def transform_event(event):
         assert "tcg-trans" not in event.properties
         assert "tcg-exec" not in event.properties
 
-        event.args = Arguments([("CPUState *", "__cpu"), event.args])
+        event.args = Arguments([("void *", "__cpu"), event.args])
         if "tcg" in event.properties:
             fmt = "\"cpu=%p \""
             event.fmt = [fmt + event.fmt[0],
-- 
2.26.1

[PATCH 3/4] build: Don't make object files for dtrace on macOS

[Roman Bolshakov]

dtrace on macOS uses unresolved symbols with a special prefix to define
probes [1], only headers should be generated for USDT (dtrace(1)). But
it doesn't support backwards compatible no-op -G flag [2] and implicit
build rules fail.

1. https://markmail.org/message/6grq2ygr5nwdwsnb
2. https://markmail.org/message/5xrxt2w5m42nojkz

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 Makefile.objs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile.objs b/Makefile.objs
index d22b3b45d7..982f15ba30 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -211,5 +211,7 @@ trace-events-files = $(SRC_PATH)/trace-events $(trace-events-subdirs:%=$(SRC_PAT
 trace-obj-y = trace-root.o
 trace-obj-y += $(trace-events-subdirs:%=%/trace.o)
 trace-obj-$(CONFIG_TRACE_UST) += trace-ust-all.o
+ifneq ($(CONFIG_DARWIN),y)
 trace-obj-$(CONFIG_TRACE_DTRACE) += trace-dtrace-root.o
 trace-obj-$(CONFIG_TRACE_DTRACE) += $(trace-events-subdirs:%=%/trace-dtrace.o)
+endif
-- 
2.26.1

[PATCH 4/4] net/colo: Match is-enabled probe to tracepoint

[Roman Bolshakov]

Build of QEMU with dtrace fails on macOS:

  LINK    x86_64-softmmu/qemu-system-x86_64
error: probe colo_compare_miscompare doesn't exist
error: Could not register probes
ld: error creating dtrace DOF section for architecture x86_64

The reason of the error is explained by Adam Leventhal [1]:

  Note that is-enabled probes don't have the stability magic so I'm not
  sure how things would work if only is-enabled probes were used.

net/colo code uses is-enabled probes to determine if other probes should
be used but colo_compare_miscompare itself is not used explicitly.
Linker doesn't include the symbol and build fails.

The issue can be resolved if is-enabled probe matches the actual trace
point that is used inside the test.

1. http://markmail.org/message/6grq2ygr5nwdwsnb

Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison")
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 net/colo-compare.c    | 12 +++++++-----
 net/filter-rewriter.c |  8 ++++++--
 net/trace-events      |  2 --
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 398b7546ff..9525ed703b 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -327,7 +327,7 @@ static int colo_compare_packet_payload(Packet *ppkt,
                                        uint16_t len)
 
 {
-    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
+    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO)) {
         char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
 
         strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src));
@@ -492,7 +492,7 @@ sec:
         g_queue_push_head(&conn->primary_list, ppkt);
         g_queue_push_head(&conn->secondary_list, spkt);
 
-        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
+        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_TCP_INFO)) {
             qemu_hexdump((char *)ppkt->data, stderr,
                         "colo-compare ppkt", ppkt->size);
             qemu_hexdump((char *)spkt->data, stderr,
@@ -533,7 +533,8 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
                                     ppkt->size - offset)) {
         trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
         trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
-        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
+        if (trace_event_get_state_backends(
+              TRACE_COLO_COMPARE_UDP_MISCOMPARE)) {
             qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
                          ppkt->size);
             qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
@@ -576,7 +577,8 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
                                            ppkt->size);
         trace_colo_compare_icmp_miscompare("Secondary pkt size",
                                            spkt->size);
-        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
+        if (trace_event_get_state_backends(
+              TRACE_COLO_COMPARE_ICMP_MISCOMPARE)) {
             qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
                          ppkt->size);
             qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
@@ -597,7 +599,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
     uint16_t offset = ppkt->vnet_hdr_len;
 
     trace_colo_compare_main("compare other");
-    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
+    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO)) {
         char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
 
         strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src));
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index 1aaad101b6..ff04165cc4 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -76,7 +76,9 @@ static int handle_primary_tcp_pkt(RewriterState *rf,
     struct tcp_hdr *tcp_pkt;
 
     tcp_pkt = (struct tcp_hdr *)pkt->transport_header;
-    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
+    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_PKT_INFO) ||
+        trace_event_get_state_backends(
+          TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
         trace_colo_filter_rewriter_pkt_info(__func__,
                     inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
                     ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
@@ -180,7 +182,9 @@ static int handle_secondary_tcp_pkt(RewriterState *rf,
 
     tcp_pkt = (struct tcp_hdr *)pkt->transport_header;
 
-    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
+    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_PKT_INFO) ||
+        trace_event_get_state_backends(
+          TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
         trace_colo_filter_rewriter_pkt_info(__func__,
                     inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
                     ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
diff --git a/net/trace-events b/net/trace-events
index fa49c71533..bfaff7891d 100644
--- a/net/trace-events
+++ b/net/trace-events
@@ -17,10 +17,8 @@ colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d"
 colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d"
 colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
 colo_old_packet_check_found(int64_t old_time) "%" PRId64
-colo_compare_miscompare(void) ""
 colo_compare_tcp_info(const char *pkt, uint32_t seq, uint32_t ack, int hdlen, int pdlen, int offset, int flags) "%s: seq/ack= %u/%u hdlen= %d pdlen= %d offset= %d flags=%d"
 
 # filter-rewriter.c
-colo_filter_rewriter_debug(void) ""
 colo_filter_rewriter_pkt_info(const char *func, const char *src, const char *dst, uint32_t seq, uint32_t ack, uint32_t flag) "%s: src/dst: %s/%s p: seq/ack=%u/%u  flags=0x%x"
 colo_filter_rewriter_conn_offset(uint32_t offset) ": offset=%u"
-- 
2.26.1

Re: [PATCH 4/4] net/colo: Match is-enabled probe to tracepoint

[Daniel P. Berrangé]

Adding Stefan to CC as the trace maintainer.

On Thu, Jul 16, 2020 at 11:17:54AM +0300, Roman Bolshakov wrote:
> Build of QEMU with dtrace fails on macOS:
> 
>   LINK    x86_64-softmmu/qemu-system-x86_64
> error: probe colo_compare_miscompare doesn't exist
> error: Could not register probes
> ld: error creating dtrace DOF section for architecture x86_64
> 
> The reason of the error is explained by Adam Leventhal [1]:
> 
>   Note that is-enabled probes don't have the stability magic so I'm not
>   sure how things would work if only is-enabled probes were used.
> 
> net/colo code uses is-enabled probes to determine if other probes should
> be used but colo_compare_miscompare itself is not used explicitly.
> Linker doesn't include the symbol and build fails.
> 
> The issue can be resolved if is-enabled probe matches the actual trace
> point that is used inside the test.
> 
> 1. http://markmail.org/message/6grq2ygr5nwdwsnb
> 
> Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison")
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  net/colo-compare.c    | 12 +++++++-----
>  net/filter-rewriter.c |  8 ++++++--
>  net/trace-events      |  2 --
>  3 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 398b7546ff..9525ed703b 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -327,7 +327,7 @@ static int colo_compare_packet_payload(Packet *ppkt,
>                                         uint16_t len)
>  
>  {
> -    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> +    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO)) {
>          char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
>  
>          strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src));
> @@ -492,7 +492,7 @@ sec:
>          g_queue_push_head(&conn->primary_list, ppkt);
>          g_queue_push_head(&conn->secondary_list, spkt);
>  
> -        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> +        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_TCP_INFO)) {
>              qemu_hexdump((char *)ppkt->data, stderr,
>                          "colo-compare ppkt", ppkt->size);
>              qemu_hexdump((char *)spkt->data, stderr,

Not your fault, as this problem is pre-existing, but IMHO this block of code
is simply broken by design. It is checking a trace event enablement state,
and then not emitting any trace event, but instead dumping info to stderr.
This is mis-use of the trace framework, and changing the event name fixes
your immediate macOS bug but the code is still flawed.

Basically it is using the trace framework as a way to dynamically turn on /
off general debugging information.

I'm not quite sure what todo to fix it, but I don't think it should be wrapped
in any trace_event_get_state_backends() call at all.

The simplest immediate option I think is to change it to be a compile time
debug

  // #define DEBUG_COLO_PACKETS

and then use

  #ifdef DEBUG_COLO_PACKETS
      qemu_hexdump(...)
  #endif

and then leave it upto the maintainer to come up with a more advanced
solution if they want to make it runtime configurable again, but not
abusing the trace framework.

> @@ -533,7 +533,8 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
>                                      ppkt->size - offset)) {
>          trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
>          trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
> -        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> +        if (trace_event_get_state_backends(
> +              TRACE_COLO_COMPARE_UDP_MISCOMPARE)) {
>              qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
>                           ppkt->size);
>              qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",

Same brokenness

> @@ -576,7 +577,8 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
>                                             ppkt->size);
>          trace_colo_compare_icmp_miscompare("Secondary pkt size",
>                                             spkt->size);
> -        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> +        if (trace_event_get_state_backends(
> +              TRACE_COLO_COMPARE_ICMP_MISCOMPARE)) {
>              qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
>                           ppkt->size);
>              qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",

Same brokenness

> @@ -597,7 +599,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
>      uint16_t offset = ppkt->vnet_hdr_len;
>  
>      trace_colo_compare_main("compare other");
> -    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> +    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO)) {
>          char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
>  
>          strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src));
> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
> index 1aaad101b6..ff04165cc4 100644
> --- a/net/filter-rewriter.c
> +++ b/net/filter-rewriter.c
> @@ -76,7 +76,9 @@ static int handle_primary_tcp_pkt(RewriterState *rf,
>      struct tcp_hdr *tcp_pkt;
>  
>      tcp_pkt = (struct tcp_hdr *)pkt->transport_header;
> -    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
> +    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_PKT_INFO) ||
> +        trace_event_get_state_backends(
> +          TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
>          trace_colo_filter_rewriter_pkt_info(__func__,
>                      inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
>                      ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),

Since there are two separate trace events here, each should be wrapped with
its own check. IOW, two completely separate if (...) trace(..); blocks

> @@ -180,7 +182,9 @@ static int handle_secondary_tcp_pkt(RewriterState *rf,
>  
>      tcp_pkt = (struct tcp_hdr *)pkt->transport_header;
>  
> -    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
> +    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_PKT_INFO) ||
> +        trace_event_get_state_backends(
> +          TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
>          trace_colo_filter_rewriter_pkt_info(__func__,
>                      inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
>                      ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),

Same here.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/4] build: Don't make object files for dtrace on macOS

[Daniel P. Berrangé]

On Thu, Jul 16, 2020 at 11:17:53AM +0300, Roman Bolshakov wrote:
> dtrace on macOS uses unresolved symbols with a special prefix to define
> probes [1], only headers should be generated for USDT (dtrace(1)). But
> it doesn't support backwards compatible no-op -G flag [2] and implicit
> build rules fail.
> 
> 1. https://markmail.org/message/6grq2ygr5nwdwsnb
> 2. https://markmail.org/message/5xrxt2w5m42nojkz
> 
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  Makefile.objs | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/4] Add dtrace support on macOS

[Daniel P. Berrangé]

Adding Stefan as the trace maintainer.

On Thu, Jul 16, 2020 at 11:17:50AM +0300, Roman Bolshakov wrote:
> Hi,
> 
> This is a small series that enables dtrace tracing backend on macOS.
> Whether or not it should go to 5.1 is up to discretion of tracing
> maintainers.
> 
> Thanks,
> Roman
> 
> Roman Bolshakov (4):
>   scripts/tracetool: Fix dtrace generation for macOS
>   scripts/tracetool: Use void pointer for vcpu
>   build: Don't make object files for dtrace on macOS
>   net/colo: Match is-enabled probe to tracepoint
> 
>  Makefile.objs                 |  2 ++
>  net/colo-compare.c            | 12 +++++++-----
>  net/filter-rewriter.c         |  8 ++++++--
>  net/trace-events              |  2 --
>  scripts/tracetool/format/d.py |  9 ++++++++-
>  scripts/tracetool/vcpu.py     |  2 +-
>  6 files changed, 24 insertions(+), 11 deletions(-)
> 
> -- 
> 2.26.1
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/4] scripts/tracetool: Fix dtrace generation for macOS

[Daniel P. Berrangé]

On Thu, Jul 16, 2020 at 11:17:51AM +0300, Roman Bolshakov wrote:
> dtrace USDT is fully supported since OS X 10.6. There are a few
> peculiarities compared to other dtrace flavors.
> 
> 1. It doesn't accept empty files.
> 2. It doesn't recognize bool type but accepts ANSI C _Bool.
> 
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  scripts/tracetool/format/d.py | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/4] scripts/tracetool: Use void pointer for vcpu

[Daniel P. Berrangé]

On Thu, Jul 16, 2020 at 11:17:52AM +0300, Roman Bolshakov wrote:
> dtrace on macOS complains that CPUState * is used for a few probes:
> 
>   dtrace: failed to compile script trace-dtrace-root.dtrace: line 130: syntax error near "CPUState"
> 
> A comment in scripts/tracetool/__init__.py mentions that:
> 
>   We only want to allow standard C types or fixed sized
>   integer types. We don't want QEMU specific types
>   as we can't assume trace backends can resolve all the
>   typedefs
> 
> Fixes: 3d211d9f4dbee ("trace: Add 'vcpu' event property to trace guest vCPU")
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  scripts/tracetool/vcpu.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/4] net/colo: Match is-enabled probe to tracepoint

[Roman Bolshakov]

On Thu, Jul 16, 2020 at 09:51:27AM +0100, Daniel P. Berrangé wrote:
> Adding Stefan to CC as the trace maintainer.
> 
> On Thu, Jul 16, 2020 at 11:17:54AM +0300, Roman Bolshakov wrote:
> > Build of QEMU with dtrace fails on macOS:
> > 
> >   LINK    x86_64-softmmu/qemu-system-x86_64
> > error: probe colo_compare_miscompare doesn't exist
> > error: Could not register probes
> > ld: error creating dtrace DOF section for architecture x86_64
> > 
> > The reason of the error is explained by Adam Leventhal [1]:
> > 
> >   Note that is-enabled probes don't have the stability magic so I'm not
> >   sure how things would work if only is-enabled probes were used.
> > 
> > net/colo code uses is-enabled probes to determine if other probes should
> > be used but colo_compare_miscompare itself is not used explicitly.
> > Linker doesn't include the symbol and build fails.
> > 
> > The issue can be resolved if is-enabled probe matches the actual trace
> > point that is used inside the test.
> > 
> > 1. http://markmail.org/message/6grq2ygr5nwdwsnb
> > 
> > Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison")
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Cc: Cameron Esfahani <dirty@apple.com>
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> >  net/colo-compare.c    | 12 +++++++-----
> >  net/filter-rewriter.c |  8 ++++++--
> >  net/trace-events      |  2 --
> >  3 files changed, 13 insertions(+), 9 deletions(-)
> > 
> > diff --git a/net/colo-compare.c b/net/colo-compare.c
> > index 398b7546ff..9525ed703b 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -327,7 +327,7 @@ static int colo_compare_packet_payload(Packet *ppkt,
> >                                         uint16_t len)
> >  
> >  {
> > -    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> > +    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO)) {
> >          char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
> >  
> >          strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src));
> > @@ -492,7 +492,7 @@ sec:
> >          g_queue_push_head(&conn->primary_list, ppkt);
> >          g_queue_push_head(&conn->secondary_list, spkt);
> >  
> > -        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> > +        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_TCP_INFO)) {
> >              qemu_hexdump((char *)ppkt->data, stderr,
> >                          "colo-compare ppkt", ppkt->size);
> >              qemu_hexdump((char *)spkt->data, stderr,
> 
> Not your fault, as this problem is pre-existing, but IMHO this block of code
> is simply broken by design. It is checking a trace event enablement state,
> and then not emitting any trace event, but instead dumping info to stderr.
> This is mis-use of the trace framework, and changing the event name fixes
> your immediate macOS bug but the code is still flawed.
> 
> Basically it is using the trace framework as a way to dynamically turn on /
> off general debugging information.
> 
> I'm not quite sure what todo to fix it, but I don't think it should be wrapped
> in any trace_event_get_state_backends() call at all.
> 
> The simplest immediate option I think is to change it to be a compile time
> debug
> 
>   // #define DEBUG_COLO_PACKETS
> 
> and then use
> 
>   #ifdef DEBUG_COLO_PACKETS
>       qemu_hexdump(...)
>   #endif
> 
> and then leave it upto the maintainer to come up with a more advanced
> solution if they want to make it runtime configurable again, but not
> abusing the trace framework.
> 


Hi Daniel,

It makes sense, compile-time define works better because the trace data
doesn't go into a trace point. I'll use that approach in v2.

> > @@ -533,7 +533,8 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
> >                                      ppkt->size - offset)) {
> >          trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> >          trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
> > -        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> > +        if (trace_event_get_state_backends(
> > +              TRACE_COLO_COMPARE_UDP_MISCOMPARE)) {
> >              qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> >                           ppkt->size);
> >              qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> 
> Same brokenness
> 
> > @@ -576,7 +577,8 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
> >                                             ppkt->size);
> >          trace_colo_compare_icmp_miscompare("Secondary pkt size",
> >                                             spkt->size);
> > -        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> > +        if (trace_event_get_state_backends(
> > +              TRACE_COLO_COMPARE_ICMP_MISCOMPARE)) {
> >              qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> >                           ppkt->size);
> >              qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> 
> Same brokenness
> 
> > @@ -597,7 +599,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
> >      uint16_t offset = ppkt->vnet_hdr_len;
> >  
> >      trace_colo_compare_main("compare other");
> > -    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> > +    if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO)) {
> >          char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
> >  
> >          strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src));
> > diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
> > index 1aaad101b6..ff04165cc4 100644
> > --- a/net/filter-rewriter.c
> > +++ b/net/filter-rewriter.c
> > @@ -76,7 +76,9 @@ static int handle_primary_tcp_pkt(RewriterState *rf,
> >      struct tcp_hdr *tcp_pkt;
> >  
> >      tcp_pkt = (struct tcp_hdr *)pkt->transport_header;
> > -    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
> > +    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_PKT_INFO) ||
> > +        trace_event_get_state_backends(
> > +          TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
> >          trace_colo_filter_rewriter_pkt_info(__func__,
> >                      inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
> >                      ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
> 
> Since there are two separate trace events here, each should be wrapped with
> its own check. IOW, two completely separate if (...) trace(..); blocks
> 


Agreed, I'll do that, thanks!

> > @@ -180,7 +182,9 @@ static int handle_secondary_tcp_pkt(RewriterState *rf,
> >  
> >      tcp_pkt = (struct tcp_hdr *)pkt->transport_header;
> >  
> > -    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
> > +    if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_PKT_INFO) ||
> > +        trace_event_get_state_backends(
> > +          TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
> >          trace_colo_filter_rewriter_pkt_info(__func__,
> >                      inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
> >                      ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
> 
> Same here.
> 
> 

Best regards,
Roman