[libvirt PATCH] wireshark: Don't include config.h

Andrea Bolognani posted 1 patch 3 years, 6 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200903104329.35960-1-abologna@redhat.com
tools/wireshark/src/packet-libvirt.c |  1 -
tools/wireshark/src/plugin.c         | 13 +++++++------
2 files changed, 7 insertions(+), 7 deletions(-)
[libvirt PATCH] wireshark: Don't include config.h
Posted by Andrea Bolognani 3 years, 6 months ago
While both Debian and Fedora include the header in their development
packages for Wireshark, that's not something that the upstream
developers intended and arguably quite wrong, as config.h is obviously
intended to only be used to drive the compilation of Wireshark itself.
The Arch Linux package behaves like the upstream Wireshark package, and
thus libvirt fails to build there.

It seems that there are multiple bugs to be addressed:

  * libvirt shouldn't include config.h;

  * Debian and Fedora shouldn't be shipping config.h in their Wireshark
    packages;

  * Wireshark should not use config.h defines such as HAVE_PLUGINS in
    its public headers, and define a public variant of them instead.

This patch takes care of the first one.

https://gitlab.com/libvirt/libvirt/-/issues/74
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 tools/wireshark/src/packet-libvirt.c |  1 -
 tools/wireshark/src/plugin.c         | 13 +++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c
index 9f3c7f650d..965f1f5482 100644
--- a/tools/wireshark/src/packet-libvirt.c
+++ b/tools/wireshark/src/packet-libvirt.c
@@ -18,7 +18,6 @@
  */
 #include <config.h>
 
-#include <wireshark/config.h>
 #include <wireshark/epan/proto.h>
 #include <wireshark/epan/packet.h>
 #include <wireshark/epan/dissectors/packet-tcp.h>
diff --git a/tools/wireshark/src/plugin.c b/tools/wireshark/src/plugin.c
index 504e4383a7..3c6fae9ef5 100644
--- a/tools/wireshark/src/plugin.c
+++ b/tools/wireshark/src/plugin.c
@@ -14,11 +14,12 @@
 
 #include <gmodule.h>
 
-#include <wireshark/config.h>
+#define HAVE_PLUGINS 1
 #include <wireshark/epan/proto.h>
 /* plugins are DLLs */
 #define WS_BUILD_DLL
 #include <wireshark/ws_symbol_export.h>
+#include <wireshark/ws_version.h>
 
 #include "packet-libvirt.h"
 
@@ -26,9 +27,9 @@
 #define PLUGIN_VERSION VERSION
 
 #define WIRESHARK_VERSION \
-    ((VERSION_MAJOR * 1000 * 1000) + \
-     (VERSION_MINOR * 1000) + \
-     (VERSION_MICRO))
+    ((WIRESHARK_VERSION_MAJOR * 1000 * 1000) + \
+     (WIRESHARK_VERSION_MINOR * 1000) + \
+     (WIRESHARK_VERSION_MICRO))
 
 #if WIRESHARK_VERSION < 2005000
 
@@ -69,8 +70,8 @@ void proto_register_libvirt(void);
 void proto_reg_handoff_libvirt(void);
 
 WS_DLL_PUBLIC_DEF const gchar plugin_version[] = PLUGIN_VERSION;
-WS_DLL_PUBLIC_DEF const int plugin_want_major = VERSION_MAJOR;
-WS_DLL_PUBLIC_DEF const int plugin_want_minor = VERSION_MINOR;
+WS_DLL_PUBLIC_DEF const int plugin_want_major = WIRESHARK_VERSION_MAJOR;
+WS_DLL_PUBLIC_DEF const int plugin_want_minor = WIRESHARK_VERSION_MINOR;
 
 WS_DLL_PUBLIC void plugin_register(void);
 
-- 
2.26.2

Re: [libvirt PATCH] wireshark: Don't include config.h
Posted by Laine Stump 3 years, 6 months ago
On 9/3/20 6:43 AM, Andrea Bolognani wrote:
> While both Debian and Fedora include the header in their development
> packages for Wireshark, that's not something that the upstream
> developers intended and arguably quite wrong, as config.h is obviously
> intended to only be used to drive the compilation of Wireshark itself.
> The Arch Linux package behaves like the upstream Wireshark package, and
> thus libvirt fails to build there.
>
> It seems that there are multiple bugs to be addressed:
>
>    * libvirt shouldn't include config.h;
>
>    * Debian and Fedora shouldn't be shipping config.h in their Wireshark
>      packages;
>
>    * Wireshark should not use config.h defines such as HAVE_PLUGINS in
>      its public headers, and define a public variant of them instead.
>
> This patch takes care of the first one.
>
> https://gitlab.com/libvirt/libvirt/-/issues/74
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>


Funny, just this morning I was grepping for " HAVE_" and " WITH_" in 
/usr/include, saw a couple of "config.h"s shoot past, and thought "Hmm. 
That doesn't seem right! Surely those packages didn't *really* intend to 
ship their config.h with their -dev package".


Reviewed-by: Laine Stump <laine@redhat.com>


> ---
>   tools/wireshark/src/packet-libvirt.c |  1 -
>   tools/wireshark/src/plugin.c         | 13 +++++++------
>   2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c
> index 9f3c7f650d..965f1f5482 100644
> --- a/tools/wireshark/src/packet-libvirt.c
> +++ b/tools/wireshark/src/packet-libvirt.c
> @@ -18,7 +18,6 @@
>    */
>   #include <config.h>
>   
> -#include <wireshark/config.h>
>   #include <wireshark/epan/proto.h>
>   #include <wireshark/epan/packet.h>
>   #include <wireshark/epan/dissectors/packet-tcp.h>
> diff --git a/tools/wireshark/src/plugin.c b/tools/wireshark/src/plugin.c
> index 504e4383a7..3c6fae9ef5 100644
> --- a/tools/wireshark/src/plugin.c
> +++ b/tools/wireshark/src/plugin.c
> @@ -14,11 +14,12 @@
>   
>   #include <gmodule.h>
>   
> -#include <wireshark/config.h>
> +#define HAVE_PLUGINS 1
>   #include <wireshark/epan/proto.h>
>   /* plugins are DLLs */
>   #define WS_BUILD_DLL
>   #include <wireshark/ws_symbol_export.h>
> +#include <wireshark/ws_version.h>
>   
>   #include "packet-libvirt.h"
>   
> @@ -26,9 +27,9 @@
>   #define PLUGIN_VERSION VERSION
>   
>   #define WIRESHARK_VERSION \
> -    ((VERSION_MAJOR * 1000 * 1000) + \
> -     (VERSION_MINOR * 1000) + \
> -     (VERSION_MICRO))
> +    ((WIRESHARK_VERSION_MAJOR * 1000 * 1000) + \
> +     (WIRESHARK_VERSION_MINOR * 1000) + \
> +     (WIRESHARK_VERSION_MICRO))
>   
>   #if WIRESHARK_VERSION < 2005000
>   
> @@ -69,8 +70,8 @@ void proto_register_libvirt(void);
>   void proto_reg_handoff_libvirt(void);
>   
>   WS_DLL_PUBLIC_DEF const gchar plugin_version[] = PLUGIN_VERSION;
> -WS_DLL_PUBLIC_DEF const int plugin_want_major = VERSION_MAJOR;
> -WS_DLL_PUBLIC_DEF const int plugin_want_minor = VERSION_MINOR;
> +WS_DLL_PUBLIC_DEF const int plugin_want_major = WIRESHARK_VERSION_MAJOR;
> +WS_DLL_PUBLIC_DEF const int plugin_want_minor = WIRESHARK_VERSION_MINOR;
>   
>   WS_DLL_PUBLIC void plugin_register(void);
>   


Re: [libvirt PATCH] wireshark: Don't include config.h
Posted by Andrea Bolognani 3 years, 6 months ago
On Thu, 2020-09-03 at 12:16 -0400, Laine Stump wrote:
> On 9/3/20 6:43 AM, Andrea Bolognani wrote:
> > While both Debian and Fedora include the header in their development
> > packages for Wireshark, that's not something that the upstream
> > developers intended and arguably quite wrong, as config.h is obviously
> > intended to only be used to drive the compilation of Wireshark itself.
> > The Arch Linux package behaves like the upstream Wireshark package, and
> > thus libvirt fails to build there.
> > 
> > It seems that there are multiple bugs to be addressed:
> > 
> >    * libvirt shouldn't include config.h;
> > 
> >    * Debian and Fedora shouldn't be shipping config.h in their Wireshark
> >      packages;
> > 
> >    * Wireshark should not use config.h defines such as HAVE_PLUGINS in
> >      its public headers, and define a public variant of them instead.
> > 
> > This patch takes care of the first one.
> > 
> > https://gitlab.com/libvirt/libvirt/-/issues/74
> > Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> 
> Funny, just this morning I was grepping for " HAVE_" and " WITH_" in 
> /usr/include, saw a couple of "config.h"s shoot past, and thought "Hmm. 
> That doesn't seem right! Surely those packages didn't *really* intend to 
> ship their config.h with their -dev package".
> 
> 
> Reviewed-by: Laine Stump <laine@redhat.com>

I should have pushed the branch to GitLab first:

  https://gitlab.com/abologna/libvirt/-/pipelines/185421025

We might be able to adopt a more nuanced approach, where we use
ws_version.h where available and fall back to config.h where it's the
only option, but I'm not quite sure how to implement that in Meson
and can't spare the time to learn right now.

Are either you or Michal interested in giving it a try?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] wireshark: Don't include config.h
Posted by Michal Prívozník 3 years, 6 months ago
On 9/4/20 10:33 AM, Andrea Bolognani wrote:
> On Thu, 2020-09-03 at 12:16 -0400, Laine Stump wrote:
>> On 9/3/20 6:43 AM, Andrea Bolognani wrote:
>>> While both Debian and Fedora include the header in their development
>>> packages for Wireshark, that's not something that the upstream
>>> developers intended and arguably quite wrong, as config.h is obviously
>>> intended to only be used to drive the compilation of Wireshark itself.
>>> The Arch Linux package behaves like the upstream Wireshark package, and
>>> thus libvirt fails to build there.
>>>
>>> It seems that there are multiple bugs to be addressed:
>>>
>>>     * libvirt shouldn't include config.h;
>>>
>>>     * Debian and Fedora shouldn't be shipping config.h in their Wireshark
>>>       packages;
>>>
>>>     * Wireshark should not use config.h defines such as HAVE_PLUGINS in
>>>       its public headers, and define a public variant of them instead.
>>>
>>> This patch takes care of the first one.
>>>
>>> https://gitlab.com/libvirt/libvirt/-/issues/74
>>> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>>
>> Funny, just this morning I was grepping for " HAVE_" and " WITH_" in
>> /usr/include, saw a couple of "config.h"s shoot past, and thought "Hmm.
>> That doesn't seem right! Surely those packages didn't *really* intend to
>> ship their config.h with their -dev package".
>>
>>
>> Reviewed-by: Laine Stump <laine@redhat.com>
> 
> I should have pushed the branch to GitLab first:
> 
>    https://gitlab.com/abologna/libvirt/-/pipelines/185421025
> 
> We might be able to adopt a more nuanced approach, where we use
> ws_version.h where available and fall back to config.h where it's the
> only option, but I'm not quite sure how to implement that in Meson
> and can't spare the time to learn right now.
> 
> Are either you or Michal interested in giving it a try?
> 

I had this patched marked for review but did not get to it yesterday, sorry.

I understand your reasoning and agree that the current code looks bad. 
But this is just the way it used to be when you wanted to built plugins 
outside of wireshark. I had a long discussion with wireshark devels when 
our plugin was being written. Part of them did not want other projects 
to build plugins from outside at all (which didn't really work for us 
because our RPC was changing a lot back then and we would have to wait 
full release cycle of wireshark to dissect new procedures), and part was 
at least willing to merge my patches that made our code less horrible.

And truth to be told, I did not really watch the discussion on wireshark 
end since and with your patch it looks they moved towards making it 
easier for other to build plugins out of wireshark's source.

End of history class.

The patch looks good. ws_version.h was introduced with 2.9.0 release 
which is 1.5 years old. Given that the dissector is aimed mostly on us, 
developers to help us debug RPC issues, I think we can safely bump the 
minimal wireshark version required (currently 2.4.0 which is 3 years old).

Michal

Re: [libvirt PATCH] wireshark: Don't include config.h
Posted by Andrea Bolognani 3 years, 6 months ago
On Fri, 2020-09-04 at 11:50 +0200, Michal Prívozník wrote:
> On 9/4/20 10:33 AM, Andrea Bolognani wrote:
> > I should have pushed the branch to GitLab first:
> > 
> >    https://gitlab.com/abologna/libvirt/-/pipelines/185421025
> > 
> > We might be able to adopt a more nuanced approach, where we use
> > ws_version.h where available and fall back to config.h where it's the
> > only option, but I'm not quite sure how to implement that in Meson
> > and can't spare the time to learn right now.
> > 
> > Are either you or Michal interested in giving it a try?
> 
> I had this patched marked for review but did not get to it yesterday, sorry.

No need to apologise :)

> I understand your reasoning and agree that the current code looks bad. 
> But this is just the way it used to be when you wanted to built plugins 
> outside of wireshark. I had a long discussion with wireshark devels when 
> our plugin was being written. Part of them did not want other projects 
> to build plugins from outside at all (which didn't really work for us 
> because our RPC was changing a lot back then and we would have to wait 
> full release cycle of wireshark to dissect new procedures), and part was 
> at least willing to merge my patches that made our code less horrible.
> 
> And truth to be told, I did not really watch the discussion on wireshark 
> end since and with your patch it looks they moved towards making it 
> easier for other to build plugins out of wireshark's source.
> 
> End of history class.
> 
> The patch looks good. ws_version.h was introduced with 2.9.0 release 
> which is 1.5 years old. Given that the dissector is aimed mostly on us, 
> developers to help us debug RPC issues, I think we can safely bump the 
> minimal wireshark version required (currently 2.4.0 which is 3 years old).

That sounds reasonable in theory, but if you look at

  https://gitlab.com/abologna/libvirt/-/pipelines/185421025

you'll see that even platforms that ship pretty recent Wireshark[1]
don't include ws_version.h among the headers.

Not building the dissector on those non-obsolete platforms seems
excessively harsh, so I think an approach similar to the one I
described above is still necessary. And at that point, you might as
well not bump the minimum required version and keep building the
dissector on the current list of platforms...


[1] Debian sid has 3.2.6 and Ubuntu 20.04 has 3.2.3
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] wireshark: Don't include config.h
Posted by Michal Prívozník 3 years, 6 months ago
On 9/4/20 2:21 PM, Andrea Bolognani wrote:
> On Fri, 2020-09-04 at 11:50 +0200, Michal Prívozník wrote:
>> On 9/4/20 10:33 AM, Andrea Bolognani wrote:
>>> I should have pushed the branch to GitLab first:
>>>
>>>     https://gitlab.com/abologna/libvirt/-/pipelines/185421025
>>>
>>> We might be able to adopt a more nuanced approach, where we use
>>> ws_version.h where available and fall back to config.h where it's the
>>> only option, but I'm not quite sure how to implement that in Meson
>>> and can't spare the time to learn right now.
>>>
>>> Are either you or Michal interested in giving it a try?
>>
>> I had this patched marked for review but did not get to it yesterday, sorry.
> 
> No need to apologise :)
> 
>> I understand your reasoning and agree that the current code looks bad.
>> But this is just the way it used to be when you wanted to built plugins
>> outside of wireshark. I had a long discussion with wireshark devels when
>> our plugin was being written. Part of them did not want other projects
>> to build plugins from outside at all (which didn't really work for us
>> because our RPC was changing a lot back then and we would have to wait
>> full release cycle of wireshark to dissect new procedures), and part was
>> at least willing to merge my patches that made our code less horrible.
>>
>> And truth to be told, I did not really watch the discussion on wireshark
>> end since and with your patch it looks they moved towards making it
>> easier for other to build plugins out of wireshark's source.
>>
>> End of history class.
>>
>> The patch looks good. ws_version.h was introduced with 2.9.0 release
>> which is 1.5 years old. Given that the dissector is aimed mostly on us,
>> developers to help us debug RPC issues, I think we can safely bump the
>> minimal wireshark version required (currently 2.4.0 which is 3 years old).
> 
> That sounds reasonable in theory, but if you look at
> 
>    https://gitlab.com/abologna/libvirt/-/pipelines/185421025
> 
> you'll see that even platforms that ship pretty recent Wireshark[1]
> don't include ws_version.h among the headers.
> 
> Not building the dissector on those non-obsolete platforms seems
> excessively harsh, so I think an approach similar to the one I
> described above is still necessary. And at that point, you might as
> well not bump the minimum required version and keep building the
> dissector on the current list of platforms...
> 
> 
> [1] Debian sid has 3.2.6 and Ubuntu 20.04 has 3.2.3

Any idea why they are not installing the file? Because while current 
solution is hacky, intentionally removing a header file that a package 
wants installed is way worse.

Michal

Re: [libvirt PATCH] wireshark: Don't include config.h
Posted by Andrea Bolognani 3 years, 6 months ago
On Fri, 2020-09-04 at 15:30 +0200, Michal Prívozník wrote:
> On 9/4/20 2:21 PM, Andrea Bolognani wrote:
> > On Fri, 2020-09-04 at 11:50 +0200, Michal Prívozník wrote:
> > > The patch looks good. ws_version.h was introduced with 2.9.0 release
> > > which is 1.5 years old. Given that the dissector is aimed mostly on us,
> > > developers to help us debug RPC issues, I think we can safely bump the
> > > minimal wireshark version required (currently 2.4.0 which is 3 years old).
> > 
> > That sounds reasonable in theory, but if you look at
> > 
> >    https://gitlab.com/abologna/libvirt/-/pipelines/185421025
> > 
> > you'll see that even platforms that ship pretty recent Wireshark[1]
> > don't include ws_version.h among the headers.
> > 
> > Not building the dissector on those non-obsolete platforms seems
> > excessively harsh, so I think an approach similar to the one I
> > described above is still necessary. And at that point, you might as
> > well not bump the minimum required version and keep building the
> > dissector on the current list of platforms...
> 
> Any idea why they are not installing the file? Because while current 
> solution is hacky, intentionally removing a header file that a package 
> wants installed is way worse.

I don't think it's done on purpose: it's probably just a bug in the
Debian packaging that got propagated to Ubuntu.

Even assuming that's the case, it will take some time for it to be
addressed in sid, and the first Ubuntu LTS that will carry the
resulting fix is almost two years out... So I think we have to just
support both ws_version.h and config.h for a while.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] wireshark: Don't include config.h
Posted by Michal Prívozník 3 years, 6 months ago
On 9/4/20 5:12 PM, Andrea Bolognani wrote:
> On Fri, 2020-09-04 at 15:30 +0200, Michal Prívozník wrote:
>> On 9/4/20 2:21 PM, Andrea Bolognani wrote:
>>> On Fri, 2020-09-04 at 11:50 +0200, Michal Prívozník wrote:
>>>> The patch looks good. ws_version.h was introduced with 2.9.0 release
>>>> which is 1.5 years old. Given that the dissector is aimed mostly on us,
>>>> developers to help us debug RPC issues, I think we can safely bump the
>>>> minimal wireshark version required (currently 2.4.0 which is 3 years old).
>>>
>>> That sounds reasonable in theory, but if you look at
>>>
>>>     https://gitlab.com/abologna/libvirt/-/pipelines/185421025
>>>
>>> you'll see that even platforms that ship pretty recent Wireshark[1]
>>> don't include ws_version.h among the headers.
>>>
>>> Not building the dissector on those non-obsolete platforms seems
>>> excessively harsh, so I think an approach similar to the one I
>>> described above is still necessary. And at that point, you might as
>>> well not bump the minimum required version and keep building the
>>> dissector on the current list of platforms...
>>
>> Any idea why they are not installing the file? Because while current
>> solution is hacky, intentionally removing a header file that a package
>> wants installed is way worse.
> 
> I don't think it's done on purpose: it's probably just a bug in the
> Debian packaging that got propagated to Ubuntu.
> 
> Even assuming that's the case, it will take some time for it to be
> addressed in sid, and the first Ubuntu LTS that will carry the
> resulting fix is almost two years out... So I think we have to just
> support both ws_version.h and config.h for a while.
> 

Ah, so on one hand we have progressive distro that doesn't install 
internal header files, on the other we have LTS distros where changing 
something may take years to take effect. In that case our only option is 
to implement both ways. Or motivate LTS distros to implement the change 
sooner ;-)

Since you wrote the original patch, do you want to write this one too? 
Or do you want me to do it?

Michal

Re: [libvirt PATCH] wireshark: Don't include config.h
Posted by Andrea Bolognani 3 years, 6 months ago
On Fri, 2020-09-04 at 17:37 +0200, Michal Prívozník wrote:
> On 9/4/20 5:12 PM, Andrea Bolognani wrote:
> > I don't think it's done on purpose: it's probably just a bug in the
> > Debian packaging that got propagated to Ubuntu.
> > 
> > Even assuming that's the case, it will take some time for it to be
> > addressed in sid, and the first Ubuntu LTS that will carry the
> > resulting fix is almost two years out... So I think we have to just
> > support both ws_version.h and config.h for a while.
> 
> Ah, so on one hand we have progressive distro that doesn't install 
> internal header files, on the other we have LTS distros where changing 
> something may take years to take effect. In that case our only option is 
> to implement both ways. Or motivate LTS distros to implement the change 
> sooner ;-)
> 
> Since you wrote the original patch, do you want to write this one too? 
> Or do you want me to do it?

I would like to see this addressed, but since there is additional
work involved it's realistically going to be a while before I can get
back to it. So, if you have any interest in writing a better patch
yourself and time to actually do it, please by all means go ahead!

-- 
Andrea Bolognani / Red Hat / Virtualization