Robin Sommer created BIT-1222:
---------------------------------
Summary: topic/robin/reader-writer-plugins
Key: BIT-1222
URL: https://bro-tracker.atlassian.net/browse/BIT-1222
Project: Bro Issue Tracker
Issue Type: Improvement
Components: Bro
Affects Versions: git/master
Reporter: Robin Sommer
This moves log writers and input readers to the new plugin API. No functional differences, except that one can now implement them via external plugins as well. Test cases for that included.
Most of the change is just moving stuff around, plus adapting to the new API. There are a few changes to defining/handling of the corresponding builtin types, as they now have to be dynamic.
--
This message was sent by Atlassian JIRA
(v6.4-OD-02-003#64000)
[ https://bro-tracker.atlassian.net/browse/BIT-1222?page=com.atlassian.jira.p… ]
Robin Sommer updated BIT-1222:
------------------------------
Status: Merge Request (was: Open)
> topic/robin/reader-writer-plugins
> ---------------------------------
>
> Key: BIT-1222
> URL: https://bro-tracker.atlassian.net/browse/BIT-1222
> Project: Bro Issue Tracker
> Issue Type: Improvement
> Components: Bro
> Affects Versions: git/master
> Reporter: Robin Sommer
>
> This moves log writers and input readers to the new plugin API. No functional differences, except that one can now implement them via external plugins as well. Test cases for that included.
> Most of the change is just moving stuff around, plus adapting to the new API. There are a few changes to defining/handling of the corresponding builtin types, as they now have to be dynamic.
--
This message was sent by Atlassian JIRA
(v6.4-OD-02-003#64000)
Open Merge Requests
===================
ID Component Reporter Assignee Updated For Version Priority Summary
------------ ----------- ------------- ------------- ---------- ------------- ---------- ---------------------------------------------------------------
BIT-1215 [1] Bro,bro-aux Daniel Thayer Daniel Thayer 2014-07-30 2.4 Normal bro-cut should be rewritten for speed and to not depend on gawk
[1] BIT-1215 https://bro-tracker.atlassian.net/browse/BIT-1215
[ https://bro-tracker.atlassian.net/browse/BIT-1220?page=com.atlassian.jira.p… ]
Jon Siwek commented on BIT-1220:
--------------------------------
It's merged now, leaving NEWS and the missing APIVersion() check for you.
> topic/robin/dynamic-plugins-2.3
> -------------------------------
>
> Key: BIT-1220
> URL: https://bro-tracker.atlassian.net/browse/BIT-1220
> Project: Bro Issue Tracker
> Issue Type: New Feature
> Components: Bro
> Affects Versions: git/master
> Reporter: Robin Sommer
> Assignee: Jon Siwek
> Fix For: 2.4
>
>
> This implements dynamic plugins for Bro, in the form of shared
> libraries loaded at startup. Tested on Linux, MacOS, and FreeBSD.
> Branches topic/robin/dynamic-plugins-2.3 are in bro, cmake, and bro-aux.
> An overview of the main functionality is in doc/devel/plugins.rst.
> This is a large change, and not everything is cast in stone yet.
> However, I think it would be good to get merged at this point to then fine-tune further later.
> I also have a few further branches based on this one that move more
> functionality over to the plugin structure (readers, writers,
> pktsrcs). I'll prepare them for merging later once this is in.
> Further notes about the code changes:
> - This removes the old Plugin macro magic, and hence touches all the
> existing analyzers to move them to the new API. Sorry. :)
> - The plugin API changed to generally use std::strings instead of
> const char*.
> - There are a number of invocations of PLUGIN_HOOK_{VOID,WITH_RESULT}
> across the code base, which allow plugins to hook into the
> processing at those locations. These are macros to make sure the
> overhead remains as low as possible when no plugin actually defines
> a hook (i.e., the normal case). See src/plugin/Manager.h for the
> macros' definition.
> - There's one hook which could be potentially expensive: plugins can
> be notified if a BroObj they are interested in gets destroyed. But I
> didn't see a performance impact in my tests (with no such hook
> defined), and the memory usage doesn't change due to field
> alignment.
> - The branch also adds a few new accessor methods to various classes
> to allow plugins to get to that information.
> - network_time cannot be just assigned to anymore, there's now
> function net_update_time() for that.
> - The branch redos how builtin variables are initialized, so that it
> works for plugins as well. No more init_net_var(), but instead
> bifcl-generated code that registers them.
> - same_type() gets an optional extra argument allowing record type
> comparision to ignore if field names don't match.
> - There are various changes for adjusting to the now dynamic
> generation of analyzer instances.
> - The file analysis API gets unified further with the protocol
> analyzer API (assigning IDs to analyzers; adding Init()/Done()
> methods; adding subtypes).
> - Adding a new command line option -Q that prints some basic execution
> time stats. Seems generally useful, and I'm planing to provide a
> plugin hook for measuring custom stuff.
> - I'm not yet happy with the current conventions for the C++
> namespaces that plugins are in. I'm planing to clean that up later
> though, as I have some more branches relying on the current scheme
> and it will be easier to clean things up once everything is in.
> - My cmake style is probably not fully consistent with the rest of the
> build system. Feel free to adapt (or also to leave as it is).
> - There's a new piece of functionality for the file analysis
> framework: activate analyzers by MIME type. Pieces going in there:
> - File::register_for_mime_type(tag: Analyzer::Tag, mt: string):
> Associates a file analyzer with a MIME type.
> - File::add_analyzers_for_mime_type(f: fa_file, mtype: string):
> Activates all analyzers registered for a MIME type for the file.
> - The default file_new() handler calls
> File::add_analyzers_for_mime_type() with the file's MIME type.
> This isn't actually used yet by any existing file analyzer (because
> we don't have any yet that would target a specific file format),
> but there's a test making sure it works.
--
This message was sent by Atlassian JIRA
(v6.4-OD-02-003#64000)
[ https://bro-tracker.atlassian.net/browse/BIT-1220?page=com.atlassian.jira.p… ]
Jon Siwek updated BIT-1220:
---------------------------
Fix Version/s: 2.4
Status: Closed (was: Merge Request)
> topic/robin/dynamic-plugins-2.3
> -------------------------------
>
> Key: BIT-1220
> URL: https://bro-tracker.atlassian.net/browse/BIT-1220
> Project: Bro Issue Tracker
> Issue Type: New Feature
> Components: Bro
> Affects Versions: git/master
> Reporter: Robin Sommer
> Assignee: Jon Siwek
> Fix For: 2.4
>
>
> This implements dynamic plugins for Bro, in the form of shared
> libraries loaded at startup. Tested on Linux, MacOS, and FreeBSD.
> Branches topic/robin/dynamic-plugins-2.3 are in bro, cmake, and bro-aux.
> An overview of the main functionality is in doc/devel/plugins.rst.
> This is a large change, and not everything is cast in stone yet.
> However, I think it would be good to get merged at this point to then fine-tune further later.
> I also have a few further branches based on this one that move more
> functionality over to the plugin structure (readers, writers,
> pktsrcs). I'll prepare them for merging later once this is in.
> Further notes about the code changes:
> - This removes the old Plugin macro magic, and hence touches all the
> existing analyzers to move them to the new API. Sorry. :)
> - The plugin API changed to generally use std::strings instead of
> const char*.
> - There are a number of invocations of PLUGIN_HOOK_{VOID,WITH_RESULT}
> across the code base, which allow plugins to hook into the
> processing at those locations. These are macros to make sure the
> overhead remains as low as possible when no plugin actually defines
> a hook (i.e., the normal case). See src/plugin/Manager.h for the
> macros' definition.
> - There's one hook which could be potentially expensive: plugins can
> be notified if a BroObj they are interested in gets destroyed. But I
> didn't see a performance impact in my tests (with no such hook
> defined), and the memory usage doesn't change due to field
> alignment.
> - The branch also adds a few new accessor methods to various classes
> to allow plugins to get to that information.
> - network_time cannot be just assigned to anymore, there's now
> function net_update_time() for that.
> - The branch redos how builtin variables are initialized, so that it
> works for plugins as well. No more init_net_var(), but instead
> bifcl-generated code that registers them.
> - same_type() gets an optional extra argument allowing record type
> comparision to ignore if field names don't match.
> - There are various changes for adjusting to the now dynamic
> generation of analyzer instances.
> - The file analysis API gets unified further with the protocol
> analyzer API (assigning IDs to analyzers; adding Init()/Done()
> methods; adding subtypes).
> - Adding a new command line option -Q that prints some basic execution
> time stats. Seems generally useful, and I'm planing to provide a
> plugin hook for measuring custom stuff.
> - I'm not yet happy with the current conventions for the C++
> namespaces that plugins are in. I'm planing to clean that up later
> though, as I have some more branches relying on the current scheme
> and it will be easier to clean things up once everything is in.
> - My cmake style is probably not fully consistent with the rest of the
> build system. Feel free to adapt (or also to leave as it is).
> - There's a new piece of functionality for the file analysis
> framework: activate analyzers by MIME type. Pieces going in there:
> - File::register_for_mime_type(tag: Analyzer::Tag, mt: string):
> Associates a file analyzer with a MIME type.
> - File::add_analyzers_for_mime_type(f: fa_file, mtype: string):
> Activates all analyzers registered for a MIME type for the file.
> - The default file_new() handler calls
> File::add_analyzers_for_mime_type() with the file's MIME type.
> This isn't actually used yet by any existing file analyzer (because
> we don't have any yet that would target a specific file format),
> but there's a test making sure it works.
--
This message was sent by Atlassian JIRA
(v6.4-OD-02-003#64000)
[ https://bro-tracker.atlassian.net/browse/BIT-1220?page=com.atlassian.jira.p… ]
Robin Sommer commented on BIT-1220:
-----------------------------------
Yeah, at a high level ("we now have a plugin interface that can do X,
Y, Z"). I can add that later if you want. I'm also planing a blog
posting once the dust has settled.
Sounds good.
It's actually used only by the BinPAC++ plugin right now, which
auto-generates Bro records that need to match Bro events, but it can
only tell field types and not names. (Independent of that, I have
wondered before if we really want the names-must-match semantics in
Bro, but that's a different topic.)
More generally, as I haven't explicitly said it (though I'm sure you
have realized :): Some of the features in this branch are driven by
what the BinPAC++/HILTI work needs. However I've tried to limit it to
things that I can see more generally useful.
Yeah, sorting should solve it.
Now you are getting ambitious. :-) Haven't though about that, but it's
probably hard because once in particular the script-level stuff is in
use, it's tricky to change it later.
I would like it to be the latter, but I'm afraid it will be the
former. The problem is that we haven't defined (yet?) what part of
Bro's ABI plugins are allowed to use. They have access to everything,
but in practice they should really limit themselves to a subset; just
what that subset that is exactly, remains unclear. For now I see the
API version mostly as way to catch some easy mistakes, but nothing
foolproof.
Oh, yes, that was there once, but seems it got lost. If you see the
right place, just add it, otherwise I can do that later.
I think we should just generally abort when a plugin is incompatible,
and then it doesn't really matter. Not sure it's worth being more
clever.
Cool, thanks for reviewing!
> topic/robin/dynamic-plugins-2.3
> -------------------------------
>
> Key: BIT-1220
> URL: https://bro-tracker.atlassian.net/browse/BIT-1220
> Project: Bro Issue Tracker
> Issue Type: New Feature
> Components: Bro
> Affects Versions: git/master
> Reporter: Robin Sommer
> Assignee: Jon Siwek
>
> This implements dynamic plugins for Bro, in the form of shared
> libraries loaded at startup. Tested on Linux, MacOS, and FreeBSD.
> Branches topic/robin/dynamic-plugins-2.3 are in bro, cmake, and bro-aux.
> An overview of the main functionality is in doc/devel/plugins.rst.
> This is a large change, and not everything is cast in stone yet.
> However, I think it would be good to get merged at this point to then fine-tune further later.
> I also have a few further branches based on this one that move more
> functionality over to the plugin structure (readers, writers,
> pktsrcs). I'll prepare them for merging later once this is in.
> Further notes about the code changes:
> - This removes the old Plugin macro magic, and hence touches all the
> existing analyzers to move them to the new API. Sorry. :)
> - The plugin API changed to generally use std::strings instead of
> const char*.
> - There are a number of invocations of PLUGIN_HOOK_{VOID,WITH_RESULT}
> across the code base, which allow plugins to hook into the
> processing at those locations. These are macros to make sure the
> overhead remains as low as possible when no plugin actually defines
> a hook (i.e., the normal case). See src/plugin/Manager.h for the
> macros' definition.
> - There's one hook which could be potentially expensive: plugins can
> be notified if a BroObj they are interested in gets destroyed. But I
> didn't see a performance impact in my tests (with no such hook
> defined), and the memory usage doesn't change due to field
> alignment.
> - The branch also adds a few new accessor methods to various classes
> to allow plugins to get to that information.
> - network_time cannot be just assigned to anymore, there's now
> function net_update_time() for that.
> - The branch redos how builtin variables are initialized, so that it
> works for plugins as well. No more init_net_var(), but instead
> bifcl-generated code that registers them.
> - same_type() gets an optional extra argument allowing record type
> comparision to ignore if field names don't match.
> - There are various changes for adjusting to the now dynamic
> generation of analyzer instances.
> - The file analysis API gets unified further with the protocol
> analyzer API (assigning IDs to analyzers; adding Init()/Done()
> methods; adding subtypes).
> - Adding a new command line option -Q that prints some basic execution
> time stats. Seems generally useful, and I'm planing to provide a
> plugin hook for measuring custom stuff.
> - I'm not yet happy with the current conventions for the C++
> namespaces that plugins are in. I'm planing to clean that up later
> though, as I have some more branches relying on the current scheme
> and it will be easier to clean things up once everything is in.
> - My cmake style is probably not fully consistent with the rest of the
> build system. Feel free to adapt (or also to leave as it is).
> - There's a new piece of functionality for the file analysis
> framework: activate analyzers by MIME type. Pieces going in there:
> - File::register_for_mime_type(tag: Analyzer::Tag, mt: string):
> Associates a file analyzer with a MIME type.
> - File::add_analyzers_for_mime_type(f: fa_file, mtype: string):
> Activates all analyzers registered for a MIME type for the file.
> - The default file_new() handler calls
> File::add_analyzers_for_mime_type() with the file's MIME type.
> This isn't actually used yet by any existing file analyzer (because
> we don't have any yet that would target a specific file format),
> but there's a test making sure it works.
--
This message was sent by Atlassian JIRA
(v6.4-OD-02-003#64000)
[ https://bro-tracker.atlassian.net/browse/BIT-1220?page=com.atlassian.jira.p… ]
Jon Siwek commented on BIT-1220:
--------------------------------
Nice code, I like the idea of the plugin-hooks.
Questions:
* NEWS worthy?
* It seemed like Plugin::Configure should be a private method and Plugin::InitPreScript, Plugin::InitPostScript, and Plugin::Done should be protected? (I've changed it already, but can revert if you think otherwise).
* Was there any code at this point that uses the new same_type() parameter for ignoring record field names? I wasn't finding any usages and I was just curious what it's for (or will be for) ?
* I think I saw some failures of testing/btest/plugins/bifs-and-scripts* and it's due to the initialization order of global plugin::__RegisterBif objects being undefined. Maybe just makes sense to sort the baselines for those tests, or do you think something more is needed?
* Any thoughts/plans for making it possible to hot-swap dynamic plugins?
* What's the general story going to look like for ABI compatibility?
** When does BRO_PLUGIN_API_VERSION get incremented? Is it when the plugin API changes, or when anything in Bro changes that breaks ABI?
** Is there a missing check of Plugin::APIVersion(); was expecting plugin manager to do it sometime after dlopen, but didn't find it?
** Currently, if a dynamic plugin is found to be incompatible, that would mean it's already run its Configure() implementation and global plugin::__RegisterBif objects were initialized? Does it need to be possible to check compatibility at an earlier stage where it will have not yet been allowed to modify anything?
Otherwise, I'll complete the merge later today or early tomorrow.
> topic/robin/dynamic-plugins-2.3
> -------------------------------
>
> Key: BIT-1220
> URL: https://bro-tracker.atlassian.net/browse/BIT-1220
> Project: Bro Issue Tracker
> Issue Type: New Feature
> Components: Bro
> Affects Versions: git/master
> Reporter: Robin Sommer
> Assignee: Jon Siwek
>
> This implements dynamic plugins for Bro, in the form of shared
> libraries loaded at startup. Tested on Linux, MacOS, and FreeBSD.
> Branches topic/robin/dynamic-plugins-2.3 are in bro, cmake, and bro-aux.
> An overview of the main functionality is in doc/devel/plugins.rst.
> This is a large change, and not everything is cast in stone yet.
> However, I think it would be good to get merged at this point to then fine-tune further later.
> I also have a few further branches based on this one that move more
> functionality over to the plugin structure (readers, writers,
> pktsrcs). I'll prepare them for merging later once this is in.
> Further notes about the code changes:
> - This removes the old Plugin macro magic, and hence touches all the
> existing analyzers to move them to the new API. Sorry. :)
> - The plugin API changed to generally use std::strings instead of
> const char*.
> - There are a number of invocations of PLUGIN_HOOK_{VOID,WITH_RESULT}
> across the code base, which allow plugins to hook into the
> processing at those locations. These are macros to make sure the
> overhead remains as low as possible when no plugin actually defines
> a hook (i.e., the normal case). See src/plugin/Manager.h for the
> macros' definition.
> - There's one hook which could be potentially expensive: plugins can
> be notified if a BroObj they are interested in gets destroyed. But I
> didn't see a performance impact in my tests (with no such hook
> defined), and the memory usage doesn't change due to field
> alignment.
> - The branch also adds a few new accessor methods to various classes
> to allow plugins to get to that information.
> - network_time cannot be just assigned to anymore, there's now
> function net_update_time() for that.
> - The branch redos how builtin variables are initialized, so that it
> works for plugins as well. No more init_net_var(), but instead
> bifcl-generated code that registers them.
> - same_type() gets an optional extra argument allowing record type
> comparision to ignore if field names don't match.
> - There are various changes for adjusting to the now dynamic
> generation of analyzer instances.
> - The file analysis API gets unified further with the protocol
> analyzer API (assigning IDs to analyzers; adding Init()/Done()
> methods; adding subtypes).
> - Adding a new command line option -Q that prints some basic execution
> time stats. Seems generally useful, and I'm planing to provide a
> plugin hook for measuring custom stuff.
> - I'm not yet happy with the current conventions for the C++
> namespaces that plugins are in. I'm planing to clean that up later
> though, as I have some more branches relying on the current scheme
> and it will be easier to clean things up once everything is in.
> - My cmake style is probably not fully consistent with the rest of the
> build system. Feel free to adapt (or also to leave as it is).
> - There's a new piece of functionality for the file analysis
> framework: activate analyzers by MIME type. Pieces going in there:
> - File::register_for_mime_type(tag: Analyzer::Tag, mt: string):
> Associates a file analyzer with a MIME type.
> - File::add_analyzers_for_mime_type(f: fa_file, mtype: string):
> Activates all analyzers registered for a MIME type for the file.
> - The default file_new() handler calls
> File::add_analyzers_for_mime_type() with the file's MIME type.
> This isn't actually used yet by any existing file analyzer (because
> we don't have any yet that would target a specific file format),
> but there's a test making sure it works.
--
This message was sent by Atlassian JIRA
(v6.4-OD-02-003#64000)
[ https://bro-tracker.atlassian.net/browse/BIT-1215?page=com.atlassian.jira.p… ]
Daniel Thayer commented on BIT-1215:
------------------------------------
In branch topic/dnthayer/ticket1215, I've made the following changes:
1) bro-cut now handles time conversion for multiple time columns in a log file (and there is a new test case),
2) bro-cut no longer has a hard-coded limit on the number of columns that it can handle,
3) all tests now pass on OS X (previously, some were failing due to strftime("%z") behavior on OS X)
> bro-cut should be rewritten for speed and to not depend on gawk
> ---------------------------------------------------------------
>
> Key: BIT-1215
> URL: https://bro-tracker.atlassian.net/browse/BIT-1215
> Project: Bro Issue Tracker
> Issue Type: Improvement
> Components: Bro, bro-aux
> Reporter: Daniel Thayer
> Assignee: Daniel Thayer
> Fix For: 2.4
>
>
> The current implementation of bro-cut is too slow when processing large log files (takes more than a minute to process a single log file a few hundred MB in size). Justin Azoff rewrote bro-cut in C and found that it runs an order of magnitude faster. Another benefit of a C version of bro-cut is that we will no longer depend on gawk for anything (and some of Bro's supported platforms do not include gawk by default).
--
This message was sent by Atlassian JIRA
(v6.4-OD-02-003#64000)
[ https://bro-tracker.atlassian.net/browse/BIT-1215?page=com.atlassian.jira.p… ]
Daniel Thayer updated BIT-1215:
-------------------------------
Status: Merge Request (was: Open)
> bro-cut should be rewritten for speed and to not depend on gawk
> ---------------------------------------------------------------
>
> Key: BIT-1215
> URL: https://bro-tracker.atlassian.net/browse/BIT-1215
> Project: Bro Issue Tracker
> Issue Type: Improvement
> Components: Bro, bro-aux
> Reporter: Daniel Thayer
> Assignee: Daniel Thayer
> Fix For: 2.4
>
>
> The current implementation of bro-cut is too slow when processing large log files (takes more than a minute to process a single log file a few hundred MB in size). Justin Azoff rewrote bro-cut in C and found that it runs an order of magnitude faster. Another benefit of a C version of bro-cut is that we will no longer depend on gawk for anything (and some of Bro's supported platforms do not include gawk by default).
--
This message was sent by Atlassian JIRA
(v6.4-OD-02-003#64000)
Open Merge Requests
===================
ID Component Reporter Assignee Updated For Version Priority Summary
------------ ----------- ------------ ---------- ---------- ------------- ---------- -----------------------------------
BIT-1220 [1] Bro Robin Sommer Jon Siwek 2014-07-28 - Normal topic/robin/dynamic-plugins-2.3 [2]
[1] BIT-1220 https://bro-tracker.atlassian.net/browse/BIT-1220
[2] dynamic-plugins-2.3 https://github.com/bro/bro/tree/topic/robin/dynamic-plugins-2.3