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
