zeek / spicy-plugin

Spicy plugin for Zeek
https://docs.zeek.org/projects/spicy/en/latest/zeek.html
Other
6 stars 9 forks source link

Enable getting spicy version from script land. #194

Closed simeonmiteff closed 1 year ago

simeonmiteff commented 1 year ago

With changes like https://github.com/zeek/spicy-plugin/pull/140, it may be useful to be able to write code like:

if (Spicy::version() < 10320)
    PacketAnalyzer::try_register_packet_analyzer_by_name("Ethernet", 0xABCD, "spicy::FOO");
    else
    PacketAnalyzer::try_register_packet_analyzer_by_name("Ethernet", 0xABCD, "spicy_FOO");

I have made a draft PR #193 to enable this:

event zeek_init() {
    print Spicy::version();
}

Output:

$ zeek test.zeek
10700
bbannier commented 1 year ago

When I saw your #193 I was wondering what a use case for this would be, so thank you for documenting one here.

The naming here and the draft PR are suggesting exposing the Spicy version while #140 would be a feature of spicy-plugin which has its own versioning. With #183 a separate version of spicy-plugin would go away, and I cannot think of anything in Spicy directly (as opposed to spicy-plugin) which would be interesting to see in Zeek-land.

From the perspective of script writing, I am still wondering whether such a version would be the right tool. Such version checks are very coarse and do not convey at all why a specific version is interesting. For your example, I would feel that the following instead would be more self-explanatory:

local analyzer_registered =
    PacketAnalyzer::try_register_packet_analyzer_by_name("Ethernet",
    0xABCD, "spicy::FOO");

# In versions before spicy-plugin-1.3.2 a different Zeek name was generated for Spicy analyzers.
if ( ! analyzer_registered )
    analyzer_registered = PacketAnalyzer::try_register_packet_analyzer_by_name(
        "Ethernet", 0xABCD, "spicy_FOO");

If one would instead care about functions or other symbols from spicy-plugin one could use existing Zeek directives like e.g., @ifdef.

I think I am still not convinced this is needed.

simeonmiteff commented 1 year ago

@bbannier thanks for looking this over. I do agree that your approach (retrying) is reasonable and has fewer moving parts.

I don't know if there are cases where zeek would abort without the opportunity to check the result of some spicy-related call (I'm guessing not).

Re: using conditional directives (@ifdef) interfere with zeek -O gen-C++, which is why I'm trying to avoid them going forward.

In the absence of a use case with no alternative, I'm closing this issue and the corresponding PR.