[libvirt] [PATCH 0/5] Couple of wireshark fixes
Well, I've just updated wireshark on my system and encountered couple of compile errors while building libvirt. Here are the fixes. Fortunately, none of them requires us to increase the version number of wireshark that's required. However, I'd like to discuss how are we going to handle this. I mean, at wireshark they don't seem so committed to API stability as we are. Therefore this patch set. In the long term I don't see us adapting to every single API change, or do I? Although I am not sure what are our options here. Michal Privoznik (5): wireshark: s/proto_tree_add_text/proto_tree_add_item/ wireshark: s/ep_alloc/wmem_alloc/ wireshark: s/tvb_length/tvb_captured_length/ wireshark: Replace WIRESHARK_COMPAT with actual version comparison wireshark: Fix header of get_message_len() tools/wireshark/src/packet-libvirt.c | 27 ++++++++++++++------------- tools/wireshark/util/genxdrstub.pl | 7 +++++-- 2 files changed, 19 insertions(+), 15 deletions(-) -- 2.4.10
In the wireshark commit e2735ecfdd7a96c they dropped proto_tree_add_text in favor of proto_tree_add_item. Adapt to this change. Moreover, the proto_tree_add_item API is around for ages and we are already using it anyway. Therefore we don't need to change required version of wireshark. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/wireshark/src/packet-libvirt.c | 2 +- tools/wireshark/util/genxdrstub.pl | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c index f7aa7ed..3103562 100644 --- a/tools/wireshark/src/packet-libvirt.c +++ b/tools/wireshark/src/packet-libvirt.c @@ -56,7 +56,7 @@ static int hf_libvirt_serial = -1; static int hf_libvirt_status = -1; static int hf_libvirt_stream = -1; static int hf_libvirt_num_of_fds = -1; -static int hf_libvirt_unknown = -1; +int hf_libvirt_unknown = -1; static gint ett_libvirt = -1; #define XDR_PRIMITIVE_DISSECTOR(xtype, ctype, ftype) \ diff --git a/tools/wireshark/util/genxdrstub.pl b/tools/wireshark/util/genxdrstub.pl index 76ccda9..07f0ff7 100755 --- a/tools/wireshark/util/genxdrstub.pl +++ b/tools/wireshark/util/genxdrstub.pl @@ -57,6 +57,9 @@ for my $proto (@ARGV) { $c->add_header_file($name, sub { dbg "*** Start parsing $proto\n"; + + $c->print("extern int hf_libvirt_unknown;\n"); + my @lexs = Lexicalizer->parse($source); for my $lex (@lexs) { next if $lex->ident eq "enum $name\_procedure"; @@ -903,7 +906,7 @@ static gboolean dissect_xdr_<%= $ident %>(tvbuff_t *tvb, proto_tree *tree, XDR * <% } %> } } else { - proto_tree_add_text(tree, tvb, start, -1, "(unknown)"); + proto_tree_add_item(tree, hf_libvirt_unknown, tvb, start, -1, ENC_NA); } return FALSE; } @@ -933,7 +936,7 @@ static gboolean dissect_xdr_<%= $ident %>(tvbuff_t *tvb, proto_tree *tree, XDR * <% } %> } if (!rc) { - proto_tree_add_text(tree, tvb, start, -1, "(unknown)"); + proto_tree_add_item(tree, hf_libvirt_unknown, tvb, start, -1, ENC_NA); } return rc; } -- 2.4.10
In wireshark, they have introduced their own memory allocator wmem. This means that we need to adapt our code to that change too. Notably 0ad15f88ccf434e8210ca is the wireshark commit you want to look at. It's the one where they dropped the old API. The new allocator has been introduced in 84cc3daa (v1.10.0), however, was not exposed until 5c05c9e0 (v1.10.0). Since we already are requiring 1.11.3 or higher no other change is needed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/wireshark/src/packet-libvirt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c index 3103562..f7c8e0c 100644 --- a/tools/wireshark/src/packet-libvirt.c +++ b/tools/wireshark/src/packet-libvirt.c @@ -113,7 +113,7 @@ format_xdr_bytes(guint8 *bytes, guint32 length) if (length == 0) return ""; - buf = ep_alloc(length*2 + 1); + buf = wmem_alloc(wmem_packet_scope(), length*2 + 1); for (i = 0; i < length; i++) { /* We know that buf has enough size to contain 2 * length + '\0' characters. */ -- 2.4.10
In wireshak commit 22149c55 (v.1.11.3) the API was renamed. Follow the change in our code too. Since the wireshark change was made in the very same version that we require at least we are good to go. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/wireshark/src/packet-libvirt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c index f7c8e0c..d0b0852 100644 --- a/tools/wireshark/src/packet-libvirt.c +++ b/tools/wireshark/src/packet-libvirt.c @@ -338,7 +338,7 @@ dissect_libvirt_payload(tvbuff_t *tvb, proto_tree *tree, { gssize payload_length; - payload_length = tvb_length(tvb) - VIR_HEADER_LEN; + payload_length = tvb_captured_length(tvb) - VIR_HEADER_LEN; if (payload_length <= 0) return; /* No payload */ @@ -405,7 +405,7 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, proto_item *ti; proto_tree *libvirt_tree; - ti = proto_tree_add_item(tree, proto_libvirt, tvb, 0, tvb_length(tvb), ENC_NA); + ti = proto_tree_add_item(tree, proto_libvirt, tvb, 0, tvb_captured_length(tvb), ENC_NA); libvirt_tree = proto_item_add_subtree(ti, ett_libvirt); offset = 0; -- 2.4.10
In the upcoming patch we will need yet another #ifdef code block depending on wireshark version. Instead of defining WIRESHARK_COMPAT2 or something lets just compare the version right at the place so that we can clearly see what version broke API. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/wireshark/src/packet-libvirt.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c index d0b0852..ac120b5 100644 --- a/tools/wireshark/src/packet-libvirt.c +++ b/tools/wireshark/src/packet-libvirt.c @@ -36,16 +36,12 @@ #include "packet-libvirt.h" #include "internal.h" +/* Wireshark 1.12 brings API change */ #define WIRESHARK_VERSION \ ((VERSION_MAJOR * 1000 * 1000) + \ (VERSION_MINOR * 1000) + \ (VERSION_MICRO)) -/* Wireshark 1.12 brings API change */ -#if WIRESHARK_VERSION < 1012000 -# define WIRESHARK_COMPAT -#endif - static int proto_libvirt = -1; static int hf_libvirt_length = -1; static int hf_libvirt_program = -1; @@ -316,7 +312,7 @@ dissect_libvirt_payload_xdr_data(tvbuff_t *tvb, proto_tree *tree, gint payload_l } payload_tvb = tvb_new_subset(tvb, start, -1, payload_length); -#ifdef WIRESHARK_COMPAT +#if WIRESHARK_VERSION < 1012000 payload_data = (caddr_t)tvb_memdup(payload_tvb, 0, payload_length); #else payload_data = (caddr_t)tvb_memdup(NULL, payload_tvb, 0, payload_length); @@ -362,7 +358,7 @@ dissect_libvirt_payload(tvbuff_t *tvb, proto_tree *tree, proto_tree_add_item(tree, hf_libvirt_unknown, tvb, VIR_HEADER_LEN, -1, ENC_NA); } -#ifdef WIRESHARK_COMPAT +#if WIRESHARK_VERSION < 1012000 static void dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree) #else @@ -430,7 +426,7 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, dissect_libvirt_payload(tvb, libvirt_tree, prog, proc, type, status); } -#ifndef WIRESHARK_COMPAT +#if WIRESHARK_VERSION >= 1012000 return 0; #endif } @@ -446,7 +442,7 @@ dissect_libvirt(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree) { /* Another magic const - 4; simply, how much bytes * is needed to tell the length of libvirt packet. */ -#ifdef WIRESHARK_COMPAT +#if WIRESHARK_VERSION < 1012000 tcp_dissect_pdus(tvb, pinfo, tree, TRUE, 4, get_message_len, dissect_libvirt_message); #else -- 2.4.10
In wireshark commit ceb8d954 (v1.99.2) they have changed the signature of a function that determines how long a libvirt packet is. Now it accepts a void pointer for passing data into the function. Well, this is nice, but we don't need it right now. Anyway, we have to change our code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/wireshark/src/packet-libvirt.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c index ac120b5..aa1c323 100644 --- a/tools/wireshark/src/packet-libvirt.c +++ b/tools/wireshark/src/packet-libvirt.c @@ -431,8 +431,13 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, #endif } +#if WIRESHARK_VERSION >= 1099002 +static guint +get_message_len(packet_info *pinfo ATTRIBUTE_UNUSED, tvbuff_t *tvb, int offset, void *data ATTRIBUTE_UNUSED) +#else static guint32 get_message_len(packet_info *pinfo ATTRIBUTE_UNUSED, tvbuff_t *tvb, int offset) +#endif { return tvb_get_ntohl(tvb, offset); } -- 2.4.10
On Mon, Jan 04, 2016 at 12:26:22PM +0100, Michal Privoznik wrote:
Well, I've just updated wireshark on my system and encountered couple of compile errors while building libvirt. Here are the fixes. Fortunately, none of them requires us to increase the version number of wireshark that's required.
However, I'd like to discuss how are we going to handle this. I mean, at wireshark they don't seem so committed to API stability as we are. Therefore this patch set. In the long term I don't see us adapting to every single API change, or do I? Although I am not sure what are our options here.
I thought about this long and hard. And we discussed it together off-list as well. And the only conclusion I could come up with is that if wireshark people don't want our dissector in, with us updating it every once in a while, then I would just say that we support few last versions (intentionally arbitrary number, so that we can drop support for older stuff whenever we please). And for those few versions we would just #ifdef the heck out of it. It's just that I don't feel like keeping all the old versions around in separate files or anything like it. I'm more than open to other ideas, though. Anyway, ACK for this series at least.
participants (2)
-
Martin Kletzander -
Michal Privoznik