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

[Michal Prívozník]

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 at 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 at 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