[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]