On Wed, Jul 29, 2020 at 03:33:22PM +0300, Roman Bolshakov wrote: > On Tue, Jul 21, 2020 at 03:06:57PM +0100, Daniel P. Berrangé wrote: > > On Sat, Jul 18, 2020 at 05:58:56PM +0000, Zhang, Chen wrote: > > > > > > > > > > -----Original Message----- > > > > From: Roman Bolshakov <r.bolsha...@yadro.com> > > > > Sent: Friday, July 17, 2020 5:35 PM > > > > To: qemu-devel@nongnu.org > > > > Cc: Daniel P. Berrangé <berra...@redhat.com>; Stefan Hajnoczi > > > > <stefa...@redhat.com>; Cameron Esfahani <di...@apple.com>; Roman > > > > Bolshakov <r.bolsha...@yadro.com>; Philippe Mathieu-Daudé > > > > <phi...@redhat.com>; Zhang, Chen <chen.zh...@intel.com>; Li Zhijian > > > > <lizhij...@cn.fujitsu.com>; Jason Wang <jasow...@redhat.com> > > > > Subject: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint > > > > > > > > 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. Packet dump toggle is replaced with a > > > > compile- > > > > time conditional definition. > > > > > > > > 1. http://markmail.org/message/6grq2ygr5nwdwsnb > > > > > > > > Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison") > > > > Cc: Philippe Mathieu-Daudé <phi...@redhat.com> > > > > Cc: Cameron Esfahani <di...@apple.com> > > > > Signed-off-by: Roman Bolshakov <r.bolsha...@yadro.com> > > > > --- > > > > net/colo-compare.c | 42 ++++++++++++++++++++++-------------------- > > > > net/filter-rewriter.c | 10 ++++++++-- > > > > net/trace-events | 2 -- > > > > 3 files changed, 30 insertions(+), 24 deletions(-) > > > > > > > > (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,12 > > > > +494,12 > > > > @@ 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)) > > > > { > > > > - qemu_hexdump((char *)ppkt->data, stderr, > > > > - "colo-compare ppkt", ppkt->size); > > > > - qemu_hexdump((char *)spkt->data, stderr, > > > > - "colo-compare spkt", spkt->size); > > > > - } > > > > +#ifdef DEBUG_COLO_PACKETS > > > > + qemu_hexdump((char *)ppkt->data, stderr, > > > > + "colo-compare ppkt", ppkt->size); > > > > + qemu_hexdump((char *)spkt->data, stderr, > > > > + "colo-compare spkt", spkt->size); #endif > > > > > > > > colo_compare_inconsistency_notify(s); > > > > } > > > > @@ -533,12 +535,12 @@ 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)) > > > > { > > > > - qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri > > > > pkt", > > > > - ppkt->size); > > > > - qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec > > > > pkt", > > > > - spkt->size); > > > > - } > > > > +#ifdef DEBUG_COLO_PACKETS > > > > + qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri > > > > pkt", > > > > + ppkt->size); > > > > + qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec > > > > pkt", > > > > + spkt->size); > > > > +#endif > > > > > > Hi Roman, > > > > > > I think change the " trace_event_get_state_backends()" to > > > "trace_colo_compare_main("Dump packet hex: ")" is a better choice here. > > > It will keep the original code logic and avoid the problem here. > > > > That may workaround the immediate bug, but this is still a misuse of the > > tracing code. Use of any trace point should only trigger actions in the > > trace infrastructure. > > > > If I'm using dtrace backend to monitor events I don't want to see QEMU > > dumping stuff to stderr. Anything written to stderr is going to trigger > > disk I/O writing to the VM's logfile, and is also liable to trigger rate > > limiting which can impact the guest performance. > > > > Hi Daniel, Chen, Stefan, > > So, what do we want to do about the series? Do we have an agreement? Is > the patch okay or I should make a change?
I think your current patch here should be merged as is, as it is removing the mis-use of the trace infrastructure. 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 :|
