From 00f93411838fd08cbcbbbf2113ef858abdb9f72d Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Wed, 5 Jun 2019 18:46:51 +0000 Subject: [PATCH 01/10] Couple of compile fixes. This is branched from topic/johanna/remove-serializer. --- src/analyzer/protocol/krb/KRB.cc | 2 ++ src/probabilistic/CounterVector.cc | 1 + 2 files changed, 3 insertions(+) diff --git a/src/analyzer/protocol/krb/KRB.cc b/src/analyzer/protocol/krb/KRB.cc index 4ee663dcf1..e6bd42b961 100644 --- a/src/analyzer/protocol/krb/KRB.cc +++ b/src/analyzer/protocol/krb/KRB.cc @@ -1,5 +1,7 @@ // See the file "COPYING" in the main distribution directory for copyright. +#include + #include "KRB.h" #include "types.bif.h" #include "events.bif.h" diff --git a/src/probabilistic/CounterVector.cc b/src/probabilistic/CounterVector.cc index 1a3c98c73f..e234d5c9d9 100644 --- a/src/probabilistic/CounterVector.cc +++ b/src/probabilistic/CounterVector.cc @@ -2,6 +2,7 @@ #include "CounterVector.h" +#include #include #include "BitVector.h" From c0c5dccd065f0568b7b9144f074c13d98045dfb1 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 7 Jun 2019 22:55:13 +0000 Subject: [PATCH 02/10] Add new test for when-statement watching global variables. --- aux/broker | 2 +- aux/btest | 2 +- aux/netcontrol-connectors | 2 +- aux/zeek-aux | 2 +- aux/zeekctl | 2 +- doc | 2 +- .../Baseline/language.when-on-globals/out | 4 ++ testing/btest/language/when-on-globals.zeek | 71 +++++++++++++++++++ 8 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 testing/btest/Baseline/language.when-on-globals/out create mode 100644 testing/btest/language/when-on-globals.zeek diff --git a/aux/broker b/aux/broker index 0c7a8816fd..e142a362b4 160000 --- a/aux/broker +++ b/aux/broker @@ -1 +1 @@ -Subproject commit 0c7a8816fd385af4f633cb7239e3c63e6c88c27e +Subproject commit e142a362b4c712699ac43494a245058948c085c8 diff --git a/aux/btest b/aux/btest index 6ece47ba64..539c2d8253 160000 --- a/aux/btest +++ b/aux/btest @@ -1 +1 @@ -Subproject commit 6ece47ba6438e7a6db5c7b85a68b3c16f0911871 +Subproject commit 539c2d82534345c62ba9a20c2e98ea5cbdea9c7e diff --git a/aux/netcontrol-connectors b/aux/netcontrol-connectors index e93235aa6e..43da5d80fd 160000 --- a/aux/netcontrol-connectors +++ b/aux/netcontrol-connectors @@ -1 +1 @@ -Subproject commit e93235aa6e45820af7e23e97627845a7b2b3d919 +Subproject commit 43da5d80fdf0923e790af3c21749f6e6241cda80 diff --git a/aux/zeek-aux b/aux/zeek-aux index 3ecc7b8c34..bac443d6ce 160000 --- a/aux/zeek-aux +++ b/aux/zeek-aux @@ -1 +1 @@ -Subproject commit 3ecc7b8c348a7b768092dad75e6cb54c6357b9d7 +Subproject commit bac443d6cebca567d3d0da52a25ff4e0bcdd1edd diff --git a/aux/zeekctl b/aux/zeekctl index a955e66c8b..37275b29f5 160000 --- a/aux/zeekctl +++ b/aux/zeekctl @@ -1 +1 @@ -Subproject commit a955e66c8b07fd6715c7ed379d0759acc592bb78 +Subproject commit 37275b29f5fe5fa68808229907a6f7bebcddafdf diff --git a/doc b/doc index e5422eafff..f8869ea5f7 160000 --- a/doc +++ b/doc @@ -1 +1 @@ -Subproject commit e5422eafff850708f4d4ff590e54299ddc97ca42 +Subproject commit f8869ea5f72cda06d5acf0fbeefd5af102f7d77e diff --git a/testing/btest/Baseline/language.when-on-globals/out b/testing/btest/Baseline/language.when-on-globals/out new file mode 100644 index 0000000000..44dae2c89e --- /dev/null +++ b/testing/btest/Baseline/language.when-on-globals/out @@ -0,0 +1,4 @@ +"j" in x3[20]$x, expected timeout +15 in x2, T +x1 != 42, T +x2[10], T diff --git a/testing/btest/language/when-on-globals.zeek b/testing/btest/language/when-on-globals.zeek new file mode 100644 index 0000000000..087a88b4db --- /dev/null +++ b/testing/btest/language/when-on-globals.zeek @@ -0,0 +1,71 @@ +# @TEST-EXEC: zeek -b -r $TRACES/wikipedia.trace %INPUT | sort >out +# @TEST-EXEC: btest-diff out + +redef exit_only_after_terminate = T; + +type X: record { + s: string; + x: set[string] &optional; +}; + +global x1 = 42; +global x2: table[count] of X; +global x3: table[count] of X; + +event quit() +{ + terminate(); +} + +event zeek_init() + { + x2[10] = [$s="foo"]; + x3[20] = [$s="bar", $x=set("i")]; + + when ( x1 != 42 ) + { + print "x1 != 42", x1 != 42; + } + timeout 1sec + { + print "unexpected timeout (1)"; + } + + when ( 15 in x2 ) + { + print "15 in x2", 10 in x2; + } + timeout 1sec + { + print "unexpected timeout (2)"; + } + + when ( x2[10]$s == "bar" ) + { + print "x2[10]", x2[10]$s == "bar"; + } + timeout 1sec + { + print "unexpected timeout (3)"; + } + + when ( "j" in x3[20]$x ) + { + print "unexpected trigger"; + } + timeout 1sec + { + print "\"j\" in x3[20]$x, expected timeout"; + } + + x1 = 100; + x2[15] = [$s="xyz"]; + x2[10]$s = "bar"; + + # This will *NOT* trigger then when-condition because we're modifying + # an inner value that's not directly tracked. + add x3[20]$x["j"]; + + schedule 2secs { quit() }; +} + From 02214dafc40cf3cf7996e879b99ec6cde1f187f1 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Thu, 6 Jun 2019 02:49:18 +0000 Subject: [PATCH 03/10] Redo NotfifierRegistry to no longer rely on StateAccess. We simplify the API to a simple Modified() operation. --- src/StateAccess.cc | 109 +++++++++++++++++++++++---------------------- src/StateAccess.h | 37 ++++++++------- src/Trigger.h | 4 +- 3 files changed, 80 insertions(+), 70 deletions(-) diff --git a/src/StateAccess.cc b/src/StateAccess.cc index 997eb5a48d..cc867468c8 100644 --- a/src/StateAccess.cc +++ b/src/StateAccess.cc @@ -149,7 +149,7 @@ void StateAccess::Replay() Val* v = target.id->ID_Val(); TypeTag t = v ? v->Type()->Tag() : TYPE_VOID; - + if ( opcode != OP_ASSIGN && ! v ) { // FIXME: I think this warrants an internal error, @@ -514,22 +514,17 @@ void StateAccess::Describe(ODesc* d) const void StateAccess::Log(StateAccess* access) { - bool tracked = false; - if ( access->target_type == TYPE_ID ) { if ( access->target.id->FindAttr(ATTR_TRACKED) ) - tracked = true; + notifiers.Modified(access->target.id); } else { if ( access->target.val->GetProperties() & MutableVal::TRACKED ) - tracked = true; + notifiers.Modified(access->target.val); } - if ( tracked ) - notifiers.AccessPerformed(*access); - #ifdef DEBUG ODesc desc; access->Describe(&desc); @@ -543,6 +538,15 @@ void StateAccess::Log(StateAccess* access) NotifierRegistry notifiers; +NotifierRegistry::~NotifierRegistry() + { + for ( auto i : ids ) + Unref(i.first); + + for ( auto i : vals ) + Unref(i.first); + } + void NotifierRegistry::Register(ID* id, NotifierRegistry::Notifier* notifier) { DBG_LOG(DBG_NOTIFIERS, "registering ID %s for notifier %s", @@ -563,24 +567,20 @@ void NotifierRegistry::Register(ID* id, NotifierRegistry::Notifier* notifier) Unref(attr); - NotifierMap::iterator i = ids.find(id->Name()); - - if ( i != ids.end() ) - i->second->insert(notifier); - else - { - NotifierSet* s = new NotifierSet; - s->insert(notifier); - ids.insert(NotifierMap::value_type(id->Name(), s)); - } - + ids.insert({id, notifier}); Ref(id); } void NotifierRegistry::Register(Val* val, NotifierRegistry::Notifier* notifier) { - if ( val->IsMutableVal() ) - Register(val->AsMutableVal()->UniqueID(), notifier); + if ( ! val->IsMutableVal() ) + return; + + DBG_LOG(DBG_NOTIFIERS, "registering value %p for notifier %s", + val, notifier->Name()); + + vals.insert({val, notifier}); + Ref(val); } void NotifierRegistry::Unregister(ID* id, NotifierRegistry::Notifier* notifier) @@ -588,52 +588,55 @@ void NotifierRegistry::Unregister(ID* id, NotifierRegistry::Notifier* notifier) DBG_LOG(DBG_NOTIFIERS, "unregistering ID %s for notifier %s", id->Name(), notifier->Name()); - NotifierMap::iterator i = ids.find(id->Name()); - - if ( i == ids.end() ) - return; - - Attr* attr = id->Attrs()->FindAttr(ATTR_TRACKED); - id->Attrs()->RemoveAttr(ATTR_TRACKED); - Unref(attr); - - NotifierSet* s = i->second; - s->erase(notifier); - - if ( s->size() == 0 ) + auto x = ids.equal_range(id); + for ( auto i = x.first; i != x.second; i++ ) { - delete s; - ids.erase(i); + if ( i->second == notifier ) + { + ids.erase(i); + Unref(id); + break; + } } - Unref(id); + if ( ids.find(id) == ids.end() ) + // Last registered notifier for this ID + id->Attrs()->RemoveAttr(ATTR_TRACKED); } void NotifierRegistry::Unregister(Val* val, NotifierRegistry::Notifier* notifier) { - if ( val->IsMutableVal() ) - Unregister(val->AsMutableVal()->UniqueID(), notifier); + DBG_LOG(DBG_NOTIFIERS, "unregistering Val %p for notifier %s", + val, notifier->Name()); + + auto x = vals.equal_range(val); + for ( auto i = x.first; i != x.second; i++ ) + { + if ( i->second == notifier ) + { + vals.erase(i); + Unref(val); + break; + } + } } -void NotifierRegistry::AccessPerformed(const StateAccess& sa) +void NotifierRegistry::Modified(Val *val) { - ID* id = sa.Target(); + DBG_LOG(DBG_NOTIFIERS, "modification to tracked value %p", val); - NotifierMap::iterator i = ids.find(id->Name()); - - if ( i == ids.end() ) - return; + auto x = vals.equal_range(val); + for ( auto i = x.first; i != x.second; i++ ) + i->second->Modified(val); + } +void NotifierRegistry::Modified(ID *id) + { DBG_LOG(DBG_NOTIFIERS, "modification to tracked ID %s", id->Name()); - NotifierSet* s = i->second; - - if ( id->IsInternalGlobal() ) - for ( NotifierSet::iterator j = s->begin(); j != s->end(); j++ ) - (*j)->Access(id->ID_Val(), sa); - else - for ( NotifierSet::iterator j = s->begin(); j != s->end(); j++ ) - (*j)->Access(id, sa); + auto x = ids.equal_range(id); + for ( auto i = x.first; i != x.second; i++ ) + i->second->Modified(id); } const char* NotifierRegistry::Notifier::Name() const diff --git a/src/StateAccess.h b/src/StateAccess.h index 15e2ae4676..86f1dbe88c 100644 --- a/src/StateAccess.h +++ b/src/StateAccess.h @@ -4,7 +4,7 @@ #define STATEACESSS_H #include -#include +#include #include class Val; @@ -105,32 +105,39 @@ public: public: virtual ~Notifier() { } - // Called when a change is being performed. Note that when these - // methods are called, it is undefined whether the change has - // already been done or is just going to be performed soon. - virtual void Access(ID* id, const StateAccess& sa) = 0; - virtual void Access(Val* val, const StateAccess& sa) = 0; + // Called when a change is being performed. Note that when + // these methods are called, it is undefined whether the + // change has already been done or is just going to be + // performed soon. + virtual void Modified(ID* id) = 0; + virtual void Modified(Val* val) = 0; virtual const char* Name() const; // for debugging }; NotifierRegistry() { } - ~NotifierRegistry() { } + ~NotifierRegistry(); - // Inform the given notifier if ID/Val changes. + // Register a new notifier to be informed when ID/Val changes. Note + // that the registry will store a reference to the target, keeping + // the instance alive for as long as it's registered. void Register(ID* id, Notifier* notifier); void Register(Val* val, Notifier* notifier); - // Cancel notification for this ID/Val. + // Cancel a notifier's tracking for this ID/Val, also releasing the + // referencee being held. void Unregister(ID* id, Notifier* notifier); void Unregister(Val* val, Notifier* notifier); -private: - friend class StateAccess; - void AccessPerformed(const StateAccess& sa); + // Inform all registered notifiiers of a modification to a value/ID. + void Modified(ID *id); + void Modified(Val *val); - typedef std::set NotifierSet; - typedef std::map NotifierMap; - NotifierMap ids; +private: + typedef std::unordered_multimap ValMap; + typedef std::unordered_multimap IDMap; + + ValMap vals; + IDMap ids; }; extern NotifierRegistry notifiers; diff --git a/src/Trigger.h b/src/Trigger.h index 0f7889d19a..8dc478e049 100644 --- a/src/Trigger.h +++ b/src/Trigger.h @@ -61,9 +61,9 @@ public: { d->Add(""); } // Overidden from Notifier. We queue the trigger and evaluate it // later to avoid race conditions. - void Access(ID* id, const StateAccess& sa) override + void Modified(ID* id) override { QueueTrigger(this); } - void Access(Val* val, const StateAccess& sa) override + void Modified(Val* val) override { QueueTrigger(this); } const char* Name() const override; From 31ddca863cf352038914415cc1e64e056914c7b2 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Thu, 6 Jun 2019 03:11:00 +0000 Subject: [PATCH 04/10] Remove StateAccess class. --- src/DebugLogger.cc | 2 +- src/DebugLogger.h | 3 +- src/ID.cc | 2 +- src/StateAccess.cc | 532 --------------------------------------------- src/StateAccess.h | 63 ------ src/Val.cc | 130 +---------- src/Val.h | 15 +- src/bro.bif | 11 - 8 files changed, 14 insertions(+), 744 deletions(-) diff --git a/src/DebugLogger.cc b/src/DebugLogger.cc index 8f0425270d..ed8b3e1e95 100644 --- a/src/DebugLogger.cc +++ b/src/DebugLogger.cc @@ -12,7 +12,7 @@ DebugLogger debug_logger; // Same order here as in DebugStream. DebugLogger::Stream DebugLogger::streams[NUM_DBGS] = { { "serial", 0, false }, { "rules", 0, false }, - { "state", 0, false }, {"string", 0, false }, + { "string", 0, false }, { "notifiers", 0, false }, { "main-loop", 0, false }, { "dpd", 0, false }, { "tm", 0, false }, { "logging", 0, false }, {"input", 0, false }, diff --git a/src/DebugLogger.h b/src/DebugLogger.h index 2e24c7064f..0e2862dc23 100644 --- a/src/DebugLogger.h +++ b/src/DebugLogger.h @@ -16,9 +16,8 @@ enum DebugStream { DBG_SERIAL, // Serialization DBG_RULES, // Signature matching - DBG_STATE, // StateAccess logging DBG_STRING, // String code - DBG_NOTIFIERS, // Notifiers (see StateAccess.h) + DBG_NOTIFIERS, // Notifiers DBG_MAINLOOP, // Main IOSource loop DBG_ANALYZER, // Analyzer framework DBG_TM, // Time-machine packet input via Brocolli diff --git a/src/ID.cc b/src/ID.cc index 12677bec75..915b5002b9 100644 --- a/src/ID.cc +++ b/src/ID.cc @@ -79,7 +79,7 @@ void ID::SetVal(Val* v, Opcode op, bool arg_weak_ref) #else if ( debug_logger.IsVerbose() || props ) #endif - StateAccess::Log(new StateAccess(op, this, v, val)); + notifiers.Modified(this); } if ( ! weak_ref ) diff --git a/src/StateAccess.cc b/src/StateAccess.cc index cc867468c8..853a8a7879 100644 --- a/src/StateAccess.cc +++ b/src/StateAccess.cc @@ -4,538 +4,6 @@ #include "NetVar.h" #include "DebugLogger.h" -int StateAccess::replaying = 0; - -StateAccess::StateAccess(Opcode arg_opcode, - const MutableVal* arg_target, const Val* arg_op1, - const Val* arg_op2, const Val* arg_op3) - { - opcode = arg_opcode; - target.val = const_cast(arg_target); - target_type = TYPE_MVAL; - op1.val = const_cast(arg_op1); - op1_type = TYPE_VAL; - op2 = const_cast(arg_op2); - op3 = const_cast(arg_op3); - delete_op1_key = false; - - RefThem(); - } - -StateAccess::StateAccess(Opcode arg_opcode, - const ID* arg_target, const Val* arg_op1, - const Val* arg_op2, const Val* arg_op3) - { - opcode = arg_opcode; - target.id = const_cast(arg_target); - target_type = TYPE_ID; - op1.val = const_cast(arg_op1); - op1_type = TYPE_VAL; - op2 = const_cast(arg_op2); - op3 = const_cast(arg_op3); - delete_op1_key = false; - - RefThem(); - } - -StateAccess::StateAccess(Opcode arg_opcode, - const ID* arg_target, const HashKey* arg_op1, - const Val* arg_op2, const Val* arg_op3) - { - opcode = arg_opcode; - target.id = const_cast(arg_target); - target_type = TYPE_ID; - op1.key = new HashKey(arg_op1->Key(), arg_op1->Size(), arg_op1->Hash()); - op1_type = TYPE_KEY; - op2 = const_cast(arg_op2); - op3 = const_cast(arg_op3); - delete_op1_key = true; - - RefThem(); - } - -StateAccess::StateAccess(Opcode arg_opcode, - const MutableVal* arg_target, const HashKey* arg_op1, - const Val* arg_op2, const Val* arg_op3) - { - opcode = arg_opcode; - target.val = const_cast(arg_target); - target_type = TYPE_MVAL; - op1.key = new HashKey(arg_op1->Key(), arg_op1->Size(), arg_op1->Hash()); - op1_type = TYPE_KEY; - op2 = const_cast(arg_op2); - op3 = const_cast(arg_op3); - delete_op1_key = true; - - RefThem(); - } - -StateAccess::StateAccess(const StateAccess& sa) - { - opcode = sa.opcode; - target_type = sa.target_type; - op1_type = sa.op1_type; - delete_op1_key = false; - - if ( target_type == TYPE_ID ) - target.id = sa.target.id; - else - target.val = sa.target.val; - - if ( op1_type == TYPE_VAL ) - op1.val = sa.op1.val; - else - { - // We need to copy the key as the pointer may not be - // valid anymore later. - op1.key = new HashKey(sa.op1.key->Key(), sa.op1.key->Size(), - sa.op1.key->Hash()); - delete_op1_key = true; - } - - op2 = sa.op2; - op3 = sa.op3; - - RefThem(); - } - -StateAccess::~StateAccess() - { - if ( target_type == TYPE_ID ) - Unref(target.id); - else - Unref(target.val); - - if ( op1_type == TYPE_VAL ) - Unref(op1.val); - else if ( delete_op1_key ) - delete op1.key; - - Unref(op2); - Unref(op3); - } - -void StateAccess::RefThem() - { - if ( target_type == TYPE_ID ) - Ref(target.id); - else - Ref(target.val); - - if ( op1_type == TYPE_VAL && op1.val ) - Ref(op1.val); - - if ( op2 ) - Ref(op2); - if ( op3 ) - Ref(op3); - } - -static Val* GetInteger(bro_int_t n, TypeTag t) - { - if ( t == TYPE_INT ) - return val_mgr->GetInt(n); - - return val_mgr->GetCount(n); - } - -void StateAccess::Replay() - { - // For simplicity we assume that we only replay unserialized accesses. - assert(target_type == TYPE_ID && op1_type == TYPE_VAL); - - if ( ! target.id ) - return; - - Val* v = target.id->ID_Val(); - TypeTag t = v ? v->Type()->Tag() : TYPE_VOID; - - if ( opcode != OP_ASSIGN && ! v ) - { - // FIXME: I think this warrants an internal error, - // but let's check that first ... - // reporter->InternalError("replay id lacking a value"); - reporter->Error("replay id lacks a value"); - return; - } - - ++replaying; - - switch ( opcode ) { - case OP_ASSIGN: - assert(op1.val); - // There mustn't be a direct assignment to a unique ID. - assert(target.id->Name()[0] != '#'); - - target.id->SetVal(op1.val->Ref()); - break; - - case OP_INCR: - if ( IsIntegral(t) ) - { - assert(op1.val && op2); - // We derive the amount as difference between old - // and new value. - bro_int_t amount = - op1.val->CoerceToInt() - op2->CoerceToInt(); - - target.id->SetVal(GetInteger(v->CoerceToInt() + amount, t), - OP_INCR); - } - break; - - case OP_ASSIGN_IDX: - assert(op1.val); - - if ( t == TYPE_TABLE ) - { - assert(op2); - v->AsTableVal()->Assign(op1.val, op2 ? op2->Ref() : 0); - } - - else if ( t == TYPE_RECORD ) - { - const char* field = op1.val->AsString()->CheckString(); - int idx = v->Type()->AsRecordType()->FieldOffset(field); - - if ( idx >= 0 ) - v->AsRecordVal()->Assign(idx, op2 ? op2->Ref() : 0); - else - reporter->Error("access replay: unknown record field %s for assign", field); - } - - else if ( t == TYPE_VECTOR ) - { - assert(op2); - bro_uint_t index = op1.val->AsCount(); - v->AsVectorVal()->Assign(index, op2 ? op2->Ref() : 0); - } - - else - reporter->InternalError("unknown type in replaying index assign"); - - break; - - case OP_INCR_IDX: - { - assert(op1.val && op2 && op3); - - // We derive the amount as the difference between old - // and new value. - bro_int_t amount = op2->CoerceToInt() - op3->CoerceToInt(); - - if ( t == TYPE_TABLE ) - { - t = v->Type()->AsTableType()->YieldType()->Tag(); - Val* lookup_op1 = v->AsTableVal()->Lookup(op1.val); - int delta = lookup_op1->CoerceToInt() + amount; - Val* new_val = GetInteger(delta, t); - v->AsTableVal()->Assign(op1.val, new_val, OP_INCR ); - } - - else if ( t == TYPE_RECORD ) - { - const char* field = op1.val->AsString()->CheckString(); - int idx = v->Type()->AsRecordType()->FieldOffset(field); - if ( idx >= 0 ) - { - t = v->Type()->AsRecordType()->FieldType(idx)->Tag(); - Val* lookup_field = - v->AsRecordVal()->Lookup(idx); - bro_int_t delta = - lookup_field->CoerceToInt() + amount; - Val* new_val = GetInteger(delta, t); - v->AsRecordVal()->Assign(idx, new_val, OP_INCR); - } - else - reporter->Error("access replay: unknown record field %s for assign", field); - } - - else if ( t == TYPE_VECTOR ) - { - bro_uint_t index = op1.val->AsCount(); - t = v->Type()->AsVectorType()->YieldType()->Tag(); - Val* lookup_op1 = v->AsVectorVal()->Lookup(index); - int delta = lookup_op1->CoerceToInt() + amount; - Val* new_val = GetInteger(delta, t); - v->AsVectorVal()->Assign(index, new_val); - } - - else - reporter->InternalError("unknown type in replaying index increment"); - - break; - } - - case OP_ADD: - assert(op1.val); - if ( t == TYPE_TABLE ) - { - v->AsTableVal()->Assign(op1.val, 0); - } - break; - - case OP_DEL: - assert(op1.val); - if ( t == TYPE_TABLE ) - { - Unref(v->AsTableVal()->Delete(op1.val)); - } - break; - - case OP_EXPIRE: - assert(op1.val); - if ( t == TYPE_TABLE ) - { - // No old check for expire. It may have already - // been deleted by ourselves. Furthermore, we - // ignore the expire_func's return value. - TableVal* tv = v->AsTableVal(); - if ( tv->Lookup(op1.val, false) ) - { - // We want to propagate state updates which - // are performed in the expire_func. - StateAccess::ResumeReplay(); - - tv->CallExpireFunc(op1.val->Ref()); - - StateAccess::SuspendReplay(); - - Unref(tv->AsTableVal()->Delete(op1.val)); - } - } - - break; - - case OP_PRINT: - assert(op1.val); - reporter->InternalError("access replay for print not implemented"); - break; - - case OP_READ_IDX: - if ( t == TYPE_TABLE ) - { - assert(op1.val); - TableVal* tv = v->AsTableVal(); - - // Update the timestamp if we have a read_expire. - if ( tv->FindAttr(ATTR_EXPIRE_READ) ) - { - tv->UpdateTimestamp(op1.val); - } - } - else - reporter->Error("read for non-table"); - break; - - default: - reporter->InternalError("access replay: unknown opcode for StateAccess"); - break; - } - - --replaying; - } - -ID* StateAccess::Target() const - { - return target_type == TYPE_ID ? target.id : target.val->UniqueID(); - } - -void StateAccess::Describe(ODesc* d) const - { - const ID* id; - const char* id_str = ""; - const char* unique_str = ""; - - d->SetShort(); - - if ( target_type == TYPE_ID ) - { - id = target.id; - - if ( ! id ) - { - d->Add("(unknown id)"); - return; - } - - id_str = id->Name(); - - if ( id->ID_Val() && id->ID_Val()->IsMutableVal() && - id->Name()[0] != '#' ) - unique_str = fmt(" [id] (%s)", id->ID_Val()->AsMutableVal()->UniqueID()->Name()); - } - else - { - id = target.val->UniqueID(); - -#ifdef DEBUG - if ( target.val->GetID() ) - { - id_str = target.val->GetID()->Name(); - unique_str = fmt(" [val] (%s)", id->Name()); - } - else -#endif - id_str = id->Name(); - } - - const Val* op1 = op1_type == TYPE_VAL ? - this->op1.val : - id->ID_Val()->AsTableVal()->RecoverIndex(this->op1.key); - - switch ( opcode ) { - case OP_ASSIGN: - assert(op1); - d->Add(id_str); - d->Add(" = "); - op1->Describe(d); - if ( op2 ) - { - d->Add(" ("); - op2->Describe(d); - d->Add(")"); - } - d->Add(unique_str); - break; - - case OP_INCR: - assert(op1 && op2); - d->Add(id_str); - d->Add(" += "); - d->Add(op1->CoerceToInt() - op2->CoerceToInt()); - d->Add(unique_str); - break; - - case OP_ASSIGN_IDX: - assert(op1); - d->Add(id_str); - d->Add("["); - op1->Describe(d); - d->Add("]"); - d->Add(" = "); - if ( op2 ) - op2->Describe(d); - else - d->Add("(null)"); - if ( op3 ) - { - d->Add(" ("); - op3->Describe(d); - d->Add(")"); - } - d->Add(unique_str); - break; - - case OP_INCR_IDX: - assert(op1 && op2 && op3); - d->Add(id_str); - d->Add("["); - op1->Describe(d); - d->Add("]"); - d->Add(" += "); - d->Add(op2->CoerceToInt() - op3->CoerceToInt()); - d->Add(unique_str); - break; - - case OP_ADD: - assert(op1); - d->Add("add "); - d->Add(id_str); - d->Add("["); - op1->Describe(d); - d->Add("]"); - if ( op2 ) - { - d->Add(" ("); - op2->Describe(d); - d->Add(")"); - } - d->Add(unique_str); - break; - - case OP_DEL: - assert(op1); - d->Add("del "); - d->Add(id_str); - d->Add("["); - op1->Describe(d); - d->Add("]"); - if ( op2 ) - { - d->Add(" ("); - op2->Describe(d); - d->Add(")"); - } - d->Add(unique_str); - break; - - case OP_EXPIRE: - assert(op1); - d->Add("expire "); - d->Add(id_str); - d->Add("["); - op1->Describe(d); - d->Add("]"); - if ( op2 ) - { - d->Add(" ("); - op2->Describe(d); - d->Add(")"); - } - d->Add(unique_str); - break; - - case OP_PRINT: - assert(op1); - d->Add("print "); - d->Add(id_str); - op1->Describe(d); - d->Add(unique_str); - break; - - case OP_READ_IDX: - assert(op1); - d->Add("read "); - d->Add(id_str); - d->Add("["); - op1->Describe(d); - d->Add("]"); - break; - - default: - reporter->InternalError("unknown opcode for StateAccess"); - break; - } - - if ( op1_type != TYPE_VAL ) - Unref(const_cast(op1)); - } - -void StateAccess::Log(StateAccess* access) - { - if ( access->target_type == TYPE_ID ) - { - if ( access->target.id->FindAttr(ATTR_TRACKED) ) - notifiers.Modified(access->target.id); - } - else - { - if ( access->target.val->GetProperties() & MutableVal::TRACKED ) - notifiers.Modified(access->target.val); - } - -#ifdef DEBUG - ODesc desc; - access->Describe(&desc); - DBG_LOG(DBG_STATE, "operation: %s%s", - desc.Description(), replaying > 0 ? " (replay)" : ""); -#endif - - delete access; - - } - NotifierRegistry notifiers; NotifierRegistry::~NotifierRegistry() diff --git a/src/StateAccess.h b/src/StateAccess.h index 86f1dbe88c..ac57d6d8f9 100644 --- a/src/StateAccess.h +++ b/src/StateAccess.h @@ -27,69 +27,6 @@ enum Opcode { // Op1 Op2 Op3 (Vals) OP_READ_IDX, // idx }; -class StateAccess { -public: - StateAccess(Opcode opcode, const ID* target, const Val* op1, - const Val* op2 = 0, const Val* op3 = 0); - StateAccess(Opcode opcode, const MutableVal* target, const Val* op1, - const Val* op2 = 0, const Val* op3 = 0); - - // For tables, the idx operand may be given as an index HashKey. - // This is for efficiency. While we need to reconstruct the index - // if we are actually going to serialize the access, we can at - // least skip it if we don't. - StateAccess(Opcode opcode, const ID* target, const HashKey* op1, - const Val* op2 = 0, const Val* op3 = 0); - StateAccess(Opcode opcode, const MutableVal* target, const HashKey* op1, - const Val* op2 = 0, const Val* op3 = 0); - - StateAccess(const StateAccess& sa); - - virtual ~StateAccess(); - - // Replays this access in the our environment. - void Replay(); - - // Returns target ID which may be an internal one for unbound vals. - ID* Target() const; - - void Describe(ODesc* d) const; - - // Main entry point when StateAcesses are performed. - // For every state-changing operation, this has to be called. - static void Log(StateAccess* access); - - // If we're going to make additional non-replaying accesses during a - // Replay(), we have to call these. - static void SuspendReplay() { --replaying; } - static void ResumeReplay() { ++replaying; } - -private: - StateAccess() { target.id = 0; op1.val = op2 = op3 = 0; } - void RefThem(); - - Opcode opcode; - union { - ID* id; - MutableVal* val; - } target; - - union { - Val* val; - const HashKey* key; - } op1; - - Val* op2; - Val* op3; - - enum Type { TYPE_ID, TYPE_VAL, TYPE_MVAL, TYPE_KEY }; - Type target_type; - Type op1_type; - bool delete_op1_key; - - static int replaying; -}; - // We provide a notifier framework to inform interested parties of // modifications to selected global IDs/Vals. To get notified about a change, // derive a class from Notifier and register the interesting IDs/Vals with diff --git a/src/Val.cc b/src/Val.cc index f0c0c031ed..61308bc3b5 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -438,8 +438,6 @@ ID* MutableVal::Bind() const "%u", ++id_counter); name[MAX_NAME_SIZE-1] = '\0'; -// DBG_LOG(DBG_STATE, "new unique ID %s", name); - id = new ID(name, SCOPE_GLOBAL, true); id->SetType(const_cast(this)->Type()->Ref()); @@ -457,8 +455,6 @@ void MutableVal::TransferUniqueID(MutableVal* mv) if ( ! id ) Bind(); - DBG_LOG(DBG_STATE, "transfering ID (new %s, old/alias %s)", new_name, id->Name()); - // Keep old name as alias. aliases.push_back(id); @@ -1178,55 +1174,6 @@ int TableVal::Assign(Val* index, HashKey* k, Val* new_val, Opcode op) subnets->Insert(index, new_entry_val); } - if ( LoggingAccess() && op != OP_NONE ) - { - Val* rec_index = 0; - if ( ! index ) - index = rec_index = RecoverIndex(&k_copy); - - if ( new_val ) - { - // A table. - if ( new_val->IsMutableVal() ) - new_val->AsMutableVal()->AddProperties(GetProperties()); - - bool unref_old_val = false; - Val* old_val = old_entry_val ? - old_entry_val->Value() : 0; - if ( op == OP_INCR && ! old_val ) - // If it's an increment, somebody has already - // checked that the index is there. If it's - // not, that can only be due to using the - // default. - { - old_val = Default(index); - unref_old_val = true; - } - - assert(op != OP_INCR || old_val); - - StateAccess::Log( - new StateAccess( - op == OP_INCR ? - OP_INCR_IDX : OP_ASSIGN_IDX, - this, index, new_val, old_val)); - - if ( unref_old_val ) - Unref(old_val); - } - - else - { - // A set. - StateAccess::Log( - new StateAccess(OP_ADD, this, - index, 0, 0)); - } - - if ( rec_index ) - Unref(rec_index); - } - // Keep old expiration time if necessary. if ( old_entry_val && attrs && attrs->FindAttr(ATTR_EXPIRE_CREATE) ) new_entry_val->SetExpireAccess(old_entry_val->ExpireAccessTime()); @@ -1237,6 +1184,7 @@ int TableVal::Assign(Val* index, HashKey* k, Val* new_val, Opcode op) delete old_entry_val; } + Modified(); return 1; } @@ -1556,11 +1504,7 @@ Val* TableVal::Lookup(Val* index, bool use_default_val) if ( v ) { if ( attrs && attrs->FindAttr(ATTR_EXPIRE_READ) ) - { v->SetExpireAccess(network_time); - if ( LoggingAccess() && ExpirationEnabled() ) - ReadOperation(index, v); - } return v->Value() ? v->Value() : this; } @@ -1587,11 +1531,7 @@ Val* TableVal::Lookup(Val* index, bool use_default_val) if ( v ) { if ( attrs && attrs->FindAttr(ATTR_EXPIRE_READ) ) - { v->SetExpireAccess(network_time); - if ( LoggingAccess() && ExpirationEnabled() ) - ReadOperation(index, v); - } return v->Value() ? v->Value() : this; } @@ -1645,11 +1585,7 @@ TableVal* TableVal::LookupSubnetValues(const SubNetVal* search) if ( entry ) { if ( attrs && attrs->FindAttr(ATTR_EXPIRE_READ) ) - { entry->SetExpireAccess(network_time); - if ( LoggingAccess() && ExpirationEnabled() ) - ReadOperation(s, entry); - } } Unref(s); // assign does not consume index @@ -1679,8 +1615,6 @@ bool TableVal::UpdateTimestamp(Val* index) return false; v->SetExpireAccess(network_time); - if ( LoggingAccess() && attrs->FindAttr(ATTR_EXPIRE_READ) ) - ReadOperation(index, v); return true; } @@ -1699,25 +1633,10 @@ Val* TableVal::Delete(const Val* index) if ( subnets && ! subnets->Remove(index) ) reporter->InternalWarning("index not in prefix table"); - if ( LoggingAccess() ) - { - if ( v ) - { - // A set. - Val* has_old_val = val_mgr->GetInt(1); - StateAccess::Log( - new StateAccess(OP_DEL, this, index, - has_old_val)); - Unref(has_old_val); - } - else - StateAccess::Log( - new StateAccess(OP_DEL, this, index, 0)); - } - delete k; delete v; + Modified(); return va; } @@ -1736,9 +1655,7 @@ Val* TableVal::Delete(const HashKey* k) delete v; - if ( LoggingAccess() ) - StateAccess::Log(new StateAccess(OP_DEL, this, k)); - + Modified(); return va; } @@ -1949,6 +1866,7 @@ void TableVal::DoExpire(double t) HashKey* k = 0; TableEntryVal* v = 0; TableEntryVal* v_saved = 0; + bool modified = false; for ( int i = 0; i < table_incremental_step && (v = tbl->NextEntry(k, expire_cookie)); ++i ) @@ -2001,18 +1919,18 @@ void TableVal::DoExpire(double t) Unref(index); } - if ( LoggingAccess() ) - StateAccess::Log( - new StateAccess(OP_EXPIRE, this, k)); - tbl->RemoveEntry(k); Unref(v->Value()); delete v; + modified = true; } delete k; } + if ( modified ) + Modified(); + if ( ! v ) { expire_cookie = 0; @@ -2124,10 +2042,7 @@ void TableVal::ReadOperation(Val* index, TableEntryVal* v) // practical issues such as latency, we send one update every half // &read_expire. if ( network_time - v->LastReadUpdate() > timeout / 2 ) - { - StateAccess::Log(new StateAccess(OP_READ_IDX, this, index)); v->SetLastReadUpdate(network_time); - } } Val* TableVal::DoClone(CloneState* state) @@ -2307,21 +2222,8 @@ RecordVal::~RecordVal() void RecordVal::Assign(int field, Val* new_val, Opcode op) { Val* old_val = AsNonConstRecord()->replace(field, new_val); - - if ( LoggingAccess() && op != OP_NONE ) - { - if ( new_val && new_val->IsMutableVal() ) - new_val->AsMutableVal()->AddProperties(GetProperties()); - - StringVal* index = new StringVal(Type()->AsRecordType()->FieldName(field)); - StateAccess::Log( - new StateAccess( - op == OP_INCR ? OP_INCR_IDX : OP_ASSIGN_IDX, - this, index, new_val, old_val)); - Unref(index); // The logging may keep a cached copy. - } - Unref(old_val); + Modified(); } Val* RecordVal::Lookup(int field) const @@ -2627,19 +2529,6 @@ bool VectorVal::Assign(unsigned int index, Val* element, Opcode op) else val.vector_val->resize(index + 1); - if ( LoggingAccess() && op != OP_NONE ) - { - if ( element->IsMutableVal() ) - element->AsMutableVal()->AddProperties(GetProperties()); - - Val* ival = val_mgr->GetCount(index); - - StateAccess::Log(new StateAccess(op == OP_INCR ? - OP_INCR_IDX : OP_ASSIGN_IDX, - this, ival, element, val_at_index)); - Unref(ival); - } - Unref(val_at_index); // Note: we do *not* Ref() the element, if any, at this point. @@ -2647,6 +2536,7 @@ bool VectorVal::Assign(unsigned int index, Val* element, Opcode op) // to do it similarly. (*val.vector_val)[index] = element; + Modified(); return true; } diff --git a/src/Val.h b/src/Val.h index 59fef19ac0..2eddac8cb7 100644 --- a/src/Val.h +++ b/src/Val.h @@ -51,8 +51,6 @@ class StringVal; class EnumVal; class MutableVal; -class StateAccess; - class VectorVal; class TableEntryVal; @@ -532,17 +530,6 @@ public: virtual bool AddProperties(Properties state); virtual bool RemoveProperties(Properties state); - // Whether StateAccess:LogAccess needs to be called. - bool LoggingAccess() const - { -#ifndef DEBUG - return props & TRACKED; -#else - return debug_logger.IsVerbose() || - (props & TRACKED); -#endif - } - protected: explicit MutableVal(BroType* t) : Val(t) { props = 0; id = 0; } @@ -553,6 +540,7 @@ protected: friend class Val; void SetID(ID* arg_id) { Unref(id); id = arg_id; } + void Modified() { notifiers.Modified(this); } private: ID* Bind() const; @@ -957,7 +945,6 @@ public: protected: friend class Val; - friend class StateAccess; TableVal() {} void Init(TableType* t); diff --git a/src/bro.bif b/src/bro.bif index 3a082bcc5f..47b713e4e1 100644 --- a/src/bro.bif +++ b/src/bro.bif @@ -1843,17 +1843,6 @@ function global_ids%(%): id_table TableVal* ids = new TableVal(id_table); PDict(ID)* globals = global_scope()->Vars(); IterCookie* c = globals->InitForIteration(); -#ifdef DEBUG - /** - * Explanation time: c needs to be a robust cookie when one is in debug mode, - * otherwise the Zeek process will crash in ~80% of cases when -B all is specified. - * The reason for this are the RecordVals that we create. RecordVal::Assign triggers - * a StateAccess::Log, which in turn (only in debug mode) triggers StateAccess::Describe, - * which creates a UniqueID for the variable, which triggers an insert into global_scope. - * Which invalidates the iteration cookie if it is not robust. - **/ - globals->MakeRobustCookie(c); -#endif ID* id; while ( (id = globals->NextEntry(c)) ) From 0ba382280c393a40672b88e00c7d9d594d31cd83 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Thu, 6 Jun 2019 03:24:13 +0000 Subject: [PATCH 05/10] Remove enum Opcode. --- src/Expr.cc | 34 +++++++++++++++++----------------- src/Expr.h | 12 ++++++------ src/ID.cc | 24 ++---------------------- src/ID.h | 2 +- src/Op.h | 23 ----------------------- src/StateAccess.h | 13 ------------- src/Val.cc | 24 +++++++++++------------- src/Val.h | 14 +++++++------- 8 files changed, 44 insertions(+), 102 deletions(-) delete mode 100644 src/Op.h diff --git a/src/Expr.cc b/src/Expr.cc index ef4d5af6e6..32cef7b0e1 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -97,7 +97,7 @@ void Expr::EvalIntoAggregate(const BroType* /* t */, Val* /* aggr */, Internal("Expr::EvalIntoAggregate called"); } -void Expr::Assign(Frame* /* f */, Val* /* v */, Opcode /* op */) +void Expr::Assign(Frame* /* f */, Val* /* v */) { Internal("Expr::Assign called"); } @@ -261,10 +261,10 @@ Expr* NameExpr::MakeLvalue() return new RefExpr(this); } -void NameExpr::Assign(Frame* f, Val* v, Opcode op) +void NameExpr::Assign(Frame* f, Val* v) { if ( id->IsGlobal() ) - id->SetVal(v, op); + id->SetVal(v); else f->SetElement(id->Offset(), v); } @@ -1007,18 +1007,18 @@ Val* IncrExpr::Eval(Frame* f) const if ( elt ) { Val* new_elt = DoSingleEval(f, elt); - v_vec->Assign(i, new_elt, OP_INCR); + v_vec->Assign(i, new_elt); } else - v_vec->Assign(i, 0, OP_INCR); + v_vec->Assign(i, 0); } - op->Assign(f, v_vec, OP_INCR); + op->Assign(f, v_vec); } else { Val* old_v = v; - op->Assign(f, v = DoSingleEval(f, old_v), OP_INCR); + op->Assign(f, v = DoSingleEval(f, old_v)); Unref(old_v); } @@ -2041,9 +2041,9 @@ Expr* RefExpr::MakeLvalue() return this; } -void RefExpr::Assign(Frame* f, Val* v, Opcode opcode) +void RefExpr::Assign(Frame* f, Val* v) { - op->Assign(f, v, opcode); + op->Assign(f, v); } AssignExpr::AssignExpr(Expr* arg_op1, Expr* arg_op2, int arg_is_init, @@ -2684,7 +2684,7 @@ Val* IndexExpr::Fold(Val* v1, Val* v2) const return 0; } -void IndexExpr::Assign(Frame* f, Val* v, Opcode op) +void IndexExpr::Assign(Frame* f, Val* v) { if ( IsError() ) return; @@ -2704,7 +2704,7 @@ void IndexExpr::Assign(Frame* f, Val* v, Opcode op) switch ( v1->Type()->Tag() ) { case TYPE_VECTOR: - if ( ! v1->AsVectorVal()->Assign(v2, v, op) ) + if ( ! v1->AsVectorVal()->Assign(v2, v) ) { if ( v ) { @@ -2723,7 +2723,7 @@ void IndexExpr::Assign(Frame* f, Val* v, Opcode op) break; case TYPE_TABLE: - if ( ! v1->AsTableVal()->Assign(v2, v, op) ) + if ( ! v1->AsTableVal()->Assign(v2, v) ) { if ( v ) { @@ -2826,7 +2826,7 @@ int FieldExpr::CanDel() const return td->FindAttr(ATTR_DEFAULT) || td->FindAttr(ATTR_OPTIONAL); } -void FieldExpr::Assign(Frame* f, Val* v, Opcode opcode) +void FieldExpr::Assign(Frame* f, Val* v) { if ( IsError() ) return; @@ -2835,14 +2835,14 @@ void FieldExpr::Assign(Frame* f, Val* v, Opcode opcode) if ( op_v ) { RecordVal* r = op_v->AsRecordVal(); - r->Assign(field, v, opcode); + r->Assign(field, v); Unref(r); } } void FieldExpr::Delete(Frame* f) { - Assign(f, 0, OP_ASSIGN_IDX); + Assign(f, 0); } Val* FieldExpr::Fold(Val* v) const @@ -4635,7 +4635,7 @@ Expr* ListExpr::MakeLvalue() return new RefExpr(this); } -void ListExpr::Assign(Frame* f, Val* v, Opcode op) +void ListExpr::Assign(Frame* f, Val* v) { ListVal* lv = v->AsListVal(); @@ -4643,7 +4643,7 @@ void ListExpr::Assign(Frame* f, Val* v, Opcode op) RuntimeError("mismatch in list lengths"); loop_over_list(exprs, i) - exprs[i]->Assign(f, (*lv->Vals())[i]->Ref(), op); + exprs[i]->Assign(f, (*lv->Vals())[i]->Ref()); Unref(lv); } diff --git a/src/Expr.h b/src/Expr.h index 6c9ac5e18f..f146162b04 100644 --- a/src/Expr.h +++ b/src/Expr.h @@ -84,7 +84,7 @@ public: const; // Assign to the given value, if appropriate. - virtual void Assign(Frame* f, Val* v, Opcode op = OP_ASSIGN); + virtual void Assign(Frame* f, Val* v); // Returns the type corresponding to this expression interpreted // as an initialization. The type should be Unref()'d when done @@ -225,7 +225,7 @@ public: ID* Id() const { return id; } Val* Eval(Frame* f) const override; - void Assign(Frame* f, Val* v, Opcode op = OP_ASSIGN) override; + void Assign(Frame* f, Val* v) override; Expr* MakeLvalue() override; int IsPure() const override; @@ -572,7 +572,7 @@ class RefExpr : public UnaryExpr { public: explicit RefExpr(Expr* op); - void Assign(Frame* f, Val* v, Opcode op = OP_ASSIGN) override; + void Assign(Frame* f, Val* v) override; Expr* MakeLvalue() override; protected: @@ -615,7 +615,7 @@ public: void Add(Frame* f) override; void Delete(Frame* f) override; - void Assign(Frame* f, Val* v, Opcode op = OP_ASSIGN) override; + void Assign(Frame* f, Val* v) override; Expr* MakeLvalue() override; // Need to override Eval since it can take a vector arg but does @@ -643,7 +643,7 @@ public: int CanDel() const override; - void Assign(Frame* f, Val* v, Opcode op = OP_ASSIGN) override; + void Assign(Frame* f, Val* v) override; void Delete(Frame* f) override; Expr* MakeLvalue() override; @@ -963,7 +963,7 @@ public: BroType* InitType() const override; Val* InitVal(const BroType* t, Val* aggr) const override; Expr* MakeLvalue() override; - void Assign(Frame* f, Val* v, Opcode op = OP_ASSIGN) override; + void Assign(Frame* f, Val* v) override; TraversalCode Traverse(TraversalCallback* cb) const override; diff --git a/src/ID.cc b/src/ID.cc index 915b5002b9..29a9a5a98a 100644 --- a/src/ID.cc +++ b/src/ID.cc @@ -59,34 +59,14 @@ void ID::ClearVal() val = 0; } -void ID::SetVal(Val* v, Opcode op, bool arg_weak_ref) +void ID::SetVal(Val* v, bool arg_weak_ref) { - if ( op != OP_NONE ) - { - MutableVal::Properties props = 0; - - if ( attrs && attrs->FindAttr(ATTR_TRACKED) ) - props |= MutableVal::TRACKED; - - if ( props ) - { - if ( v->IsMutableVal() ) - v->AsMutableVal()->AddProperties(props); - } - -#ifndef DEBUG - if ( props ) -#else - if ( debug_logger.IsVerbose() || props ) -#endif - notifiers.Modified(this); - } - if ( ! weak_ref ) Unref(val); val = v; weak_ref = arg_weak_ref; + notifiers.Modified(this); #ifdef DEBUG UpdateValID(); diff --git a/src/ID.h b/src/ID.h index bb9e11ca06..7717a584f3 100644 --- a/src/ID.h +++ b/src/ID.h @@ -46,7 +46,7 @@ public: // reference to the Val, the Val will be destroyed (naturally, // you have to take care that it will not be accessed via // the ID afterwards). - void SetVal(Val* v, Opcode op = OP_ASSIGN, bool weak_ref = false); + void SetVal(Val* v, bool weak_ref = false); void SetVal(Val* v, init_class c); void SetVal(Expr* ev, init_class c); diff --git a/src/Op.h b/src/Op.h deleted file mode 100644 index a628a6bb68..0000000000 --- a/src/Op.h +++ /dev/null @@ -1,23 +0,0 @@ -// See the file "COPYING" in the main distribution directory for copyright. - -#ifndef op_h -#define op_h - -// BRO operations. - -typedef enum { - OP_INCR, OP_DECR, OP_NOT, OP_NEGATE, - OP_PLUS, OP_MINUS, OP_TIMES, OP_DIVIDE, OP_MOD, - OP_AND, OP_OR, - OP_LT, OP_LE, OP_EQ, OP_NE, OP_GE, OP_GT, - OP_MATCH, - OP_ASSIGN, - OP_INDEX, OP_FIELD, - OP_IN, - OP_LIST, - OP_CALL, - OP_SCHED, - OP_NAME, OP_CONST, OP_THIS -} BroOP; - -#endif diff --git a/src/StateAccess.h b/src/StateAccess.h index ac57d6d8f9..528e3357d5 100644 --- a/src/StateAccess.h +++ b/src/StateAccess.h @@ -14,19 +14,6 @@ class HashKey; class ODesc; class TableVal; -enum Opcode { // Op1 Op2 Op3 (Vals) - OP_NONE, - OP_ASSIGN, // new old - OP_ASSIGN_IDX, // idx new old - OP_ADD, // idx old - OP_INCR, // idx new old - OP_INCR_IDX, // idx new old - OP_DEL, // idx old - OP_PRINT, // args - OP_EXPIRE, // idx - OP_READ_IDX, // idx -}; - // We provide a notifier framework to inform interested parties of // modifications to selected global IDs/Vals. To get notified about a change, // derive a class from Notifier and register the interesting IDs/Vals with diff --git a/src/Val.cc b/src/Val.cc index 61308bc3b5..75f6f9134d 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -443,7 +443,7 @@ ID* MutableVal::Bind() const global_scope()->Insert(name, id); - id->SetVal(const_cast(this), OP_NONE, true); + id->SetVal(const_cast(this), true); return id; } @@ -461,7 +461,7 @@ void MutableVal::TransferUniqueID(MutableVal* mv) id = new ID(new_name, SCOPE_GLOBAL, true); id->SetType(const_cast(this)->Type()->Ref()); global_scope()->Insert(new_name, id); - id->SetVal(const_cast(this), OP_NONE, true); + id->SetVal(const_cast(this), true); Unref(mv->id); mv->id = 0; @@ -1132,7 +1132,7 @@ void TableVal::CheckExpireAttr(attr_tag at) } } -int TableVal::Assign(Val* index, Val* new_val, Opcode op) +int TableVal::Assign(Val* index, Val* new_val) { HashKey* k = ComputeHash(index); if ( ! k ) @@ -1142,10 +1142,10 @@ int TableVal::Assign(Val* index, Val* new_val, Opcode op) return 0; } - return Assign(index, k, new_val, op); + return Assign(index, k, new_val); } -int TableVal::Assign(Val* index, HashKey* k, Val* new_val, Opcode op) +int TableVal::Assign(Val* index, HashKey* k, Val* new_val) { int is_set = table_type->IsSet(); @@ -1227,15 +1227,13 @@ int TableVal::AddTo(Val* val, int is_first_init, bool propagate_ops) const if ( type->IsSet() ) { - if ( ! t->Assign(v->Value(), k, 0, - propagate_ops ? OP_ASSIGN : OP_NONE) ) + if ( ! t->Assign(v->Value(), k, 0) ) return 0; } else { v->Ref(); - if ( ! t->Assign(0, k, v->Value(), - propagate_ops ? OP_ASSIGN : OP_NONE) ) + if ( ! t->Assign(0, k, v->Value()) ) return 0; } } @@ -1822,7 +1820,7 @@ int TableVal::ExpandCompoundAndInit(val_list* vl, int k, Val* new_val) return 1; } -int TableVal::CheckAndAssign(Val* index, Val* new_val, Opcode op) +int TableVal::CheckAndAssign(Val* index, Val* new_val) { Val* v = 0; if ( subnets ) @@ -1834,7 +1832,7 @@ int TableVal::CheckAndAssign(Val* index, Val* new_val, Opcode op) if ( v ) index->Warn("multiple initializations for index"); - return Assign(index, new_val, op); + return Assign(index, new_val); } void TableVal::InitTimer(double delay) @@ -2219,7 +2217,7 @@ RecordVal::~RecordVal() delete_vals(AsNonConstRecord()); } -void RecordVal::Assign(int field, Val* new_val, Opcode op) +void RecordVal::Assign(int field, Val* new_val) { Val* old_val = AsNonConstRecord()->replace(field, new_val); Unref(old_val); @@ -2513,7 +2511,7 @@ VectorVal::~VectorVal() delete val.vector_val; } -bool VectorVal::Assign(unsigned int index, Val* element, Opcode op) +bool VectorVal::Assign(unsigned int index, Val* element) { if ( element && ! same_type(element->Type(), vector_type->YieldType(), 0) ) diff --git a/src/Val.h b/src/Val.h index 2eddac8cb7..08038a686b 100644 --- a/src/Val.h +++ b/src/Val.h @@ -840,8 +840,8 @@ public: // version takes a HashKey and Unref()'s it when done. If we're a // set, new_val has to be nil. If we aren't a set, index may be nil // in the second version. - int Assign(Val* index, Val* new_val, Opcode op = OP_ASSIGN); - int Assign(Val* index, HashKey* k, Val* new_val, Opcode op = OP_ASSIGN); + int Assign(Val* index, Val* new_val); + int Assign(Val* index, HashKey* k, Val* new_val); Val* SizeVal() const override { return val_mgr->GetCount(Size()); } @@ -951,7 +951,7 @@ protected: void CheckExpireAttr(attr_tag at); int ExpandCompoundAndInit(val_list* vl, int k, Val* new_val); - int CheckAndAssign(Val* index, Val* new_val, Opcode op = OP_ASSIGN); + int CheckAndAssign(Val* index, Val* new_val); bool AddProperties(Properties arg_state) override; bool RemoveProperties(Properties arg_state) override; @@ -995,7 +995,7 @@ public: Val* SizeVal() const override { return val_mgr->GetCount(record_type->NumFields()); } - void Assign(int field, Val* new_val, Opcode op = OP_ASSIGN); + void Assign(int field, Val* new_val); Val* Lookup(int field) const; // Does not Ref() value. Val* LookupWithDefault(int field) const; // Does Ref() value. @@ -1095,11 +1095,11 @@ public: // Note: does NOT Ref() the element! Remember to do so unless // the element was just created and thus has refcount 1. // - bool Assign(unsigned int index, Val* element, Opcode op = OP_ASSIGN); - bool Assign(Val* index, Val* element, Opcode op = OP_ASSIGN) + bool Assign(unsigned int index, Val* element); + bool Assign(Val* index, Val* element) { return Assign(index->AsListVal()->Index(0)->CoerceToUnsigned(), - element, op); + element); } // Assigns the value to how_many locations starting at index. From f8262b65c42aacdf4234b61e80d27726768956b4 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Thu, 6 Jun 2019 03:28:12 +0000 Subject: [PATCH 06/10] Remove most of MutableVal (but not the class itelf yet) --- src/ID.cc | 20 ----- src/Val.cc | 239 ----------------------------------------------------- src/Val.h | 91 ++------------------ 3 files changed, 7 insertions(+), 343 deletions(-) diff --git a/src/ID.cc b/src/ID.cc index 29a9a5a98a..582c8fcf8e 100644 --- a/src/ID.cc +++ b/src/ID.cc @@ -155,16 +155,6 @@ void ID::UpdateValAttrs() if ( ! attrs ) return; - MutableVal::Properties props = 0; - - if ( val && val->IsMutableVal() ) - { - if ( attrs->FindAttr(ATTR_TRACKED) ) - props |= MutableVal::TRACKED; - - val->AsMutableVal()->AddProperties(props); - } - if ( val && val->Type()->Tag() == TYPE_TABLE ) val->AsTableVal()->SetAttrs(attrs); @@ -222,16 +212,6 @@ void ID::RemoveAttr(attr_tag a) { if ( attrs ) attrs->RemoveAttr(a); - - if ( val && val->IsMutableVal() ) - { - MutableVal::Properties props = 0; - - if ( a == ATTR_TRACKED ) - props |= MutableVal::TRACKED; - - val->AsMutableVal()->RemoveProperties(props); - } } void ID::SetOption() diff --git a/src/Val.cc b/src/Val.cc index 75f6f9134d..4ba2ea6dd7 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -351,120 +351,6 @@ void Val::ValDescribeReST(ODesc* d) const MutableVal::~MutableVal() { - for ( list::iterator i = aliases.begin(); i != aliases.end(); ++i ) - { - if ( global_scope() ) - global_scope()->Remove((*i)->Name()); - (*i)->ClearVal(); // just to make sure. - Unref((*i)); - } - - if ( id ) - { - if ( global_scope() ) - global_scope()->Remove(id->Name()); - id->ClearVal(); // just to make sure. - Unref(id); - } - } - -bool MutableVal::AddProperties(Properties arg_props) - { - if ( (props | arg_props) == props ) - // No change. - return false; - - props |= arg_props; - - if ( ! id ) - Bind(); - - return true; - } - - -bool MutableVal::RemoveProperties(Properties arg_props) - { - if ( (props & ~arg_props) == props ) - // No change. - return false; - - props &= ~arg_props; - - return true; - } - -ID* MutableVal::Bind() const - { - static bool initialized = false; - - assert(!id); - - static unsigned int id_counter = 0; - static const int MAX_NAME_SIZE = 128; - static char name[MAX_NAME_SIZE]; - static char* end_of_static_str = 0; - - if ( ! initialized ) - { - // Get local IP. - char host[MAXHOSTNAMELEN]; - strcpy(host, "localhost"); - gethostname(host, MAXHOSTNAMELEN); - host[MAXHOSTNAMELEN-1] = '\0'; -#if 0 - // We ignore errors. - struct hostent* ent = gethostbyname(host); - - uint32 ip; - if ( ent && ent->h_addr_list[0] ) - ip = *(uint32*) ent->h_addr_list[0]; - else - ip = htonl(0x7f000001); // 127.0.0.1 - - safe_snprintf(name, MAX_NAME_SIZE, "#%s#%d#", - IPAddr(IPv4, &ip, IPAddr::Network)->AsString().c_str(), - getpid()); -#else - safe_snprintf(name, MAX_NAME_SIZE, "#%s#%d#", host, getpid()); -#endif - - end_of_static_str = name + strlen(name); - - initialized = true; - } - - safe_snprintf(end_of_static_str, MAX_NAME_SIZE - (end_of_static_str - name), - "%u", ++id_counter); - name[MAX_NAME_SIZE-1] = '\0'; - - id = new ID(name, SCOPE_GLOBAL, true); - id->SetType(const_cast(this)->Type()->Ref()); - - global_scope()->Insert(name, id); - - id->SetVal(const_cast(this), true); - - return id; - } - -void MutableVal::TransferUniqueID(MutableVal* mv) - { - const char* new_name = mv->UniqueID()->Name(); - - if ( ! id ) - Bind(); - - // Keep old name as alias. - aliases.push_back(id); - - id = new ID(new_name, SCOPE_GLOBAL, true); - id->SetType(const_cast(this)->Type()->Ref()); - global_scope()->Insert(new_name, id); - id->SetVal(const_cast(this), true); - - Unref(mv->id); - mv->id = 0; } IntervalVal::IntervalVal(double quantity, double units) : @@ -2026,23 +1912,6 @@ double TableVal::CallExpireFunc(Val* idx) return secs; } -void TableVal::ReadOperation(Val* index, TableEntryVal* v) - { - double timeout = GetExpireTime(); - - if ( timeout < 0 ) - // Skip in case of unset/invalid expiration value. If it's an - // error, it has been reported already. - return; - - // In theory we need to only propagate one update per &read_expire - // interval to prevent peers from expiring intervals. To account for - // practical issues such as latency, we send one update every half - // &read_expire. - if ( network_time - v->LastReadUpdate() > timeout / 2 ) - v->SetLastReadUpdate(network_time); - } - Val* TableVal::DoClone(CloneState* state) { auto tv = new TableVal(table_type); @@ -2092,48 +1961,6 @@ Val* TableVal::DoClone(CloneState* state) return tv; } -bool TableVal::AddProperties(Properties arg_props) - { - if ( ! MutableVal::AddProperties(arg_props) ) - return false; - - if ( Type()->IsSet() || ! RecursiveProps(arg_props) ) - return true; - - // For a large table, this could get expensive. So, let's hope - // that nobody creates such a table *before* making it persistent - // (for example by inserting it into another table). - TableEntryVal* v; - PDict(TableEntryVal)* tbl = val.table_val; - IterCookie* c = tbl->InitForIteration(); - while ( (v = tbl->NextEntry(c)) ) - if ( v->Value()->IsMutableVal() ) - v->Value()->AsMutableVal()->AddProperties(RecursiveProps(arg_props)); - - return true; - } - -bool TableVal::RemoveProperties(Properties arg_props) - { - if ( ! MutableVal::RemoveProperties(arg_props) ) - return false; - - if ( Type()->IsSet() || ! RecursiveProps(arg_props) ) - return true; - - // For a large table, this could get expensive. So, let's hope - // that nobody creates such a table *before* making it persistent - // (for example by inserting it into another table). - TableEntryVal* v; - PDict(TableEntryVal)* tbl = val.table_val; - IterCookie* c = tbl->InitForIteration(); - while ( (v = tbl->NextEntry(c)) ) - if ( v->Value()->IsMutableVal() ) - v->Value()->AsMutableVal()->RemoveProperties(RecursiveProps(arg_props)); - - return true; - } - unsigned int TableVal::MemoryAllocation() const { unsigned int size = 0; @@ -2428,41 +2255,6 @@ Val* RecordVal::DoClone(CloneState* state) return rv; } -bool RecordVal::AddProperties(Properties arg_props) - { - if ( ! MutableVal::AddProperties(arg_props) ) - return false; - - if ( ! RecursiveProps(arg_props) ) - return true; - - loop_over_list(*val.val_list_val, i) - { - Val* v = (*val.val_list_val)[i]; - if ( v && v->IsMutableVal() ) - v->AsMutableVal()->AddProperties(RecursiveProps(arg_props)); - } - return true; - } - - -bool RecordVal::RemoveProperties(Properties arg_props) - { - if ( ! MutableVal::RemoveProperties(arg_props) ) - return false; - - if ( ! RecursiveProps(arg_props) ) - return true; - - loop_over_list(*val.val_list_val, i) - { - Val* v = (*val.val_list_val)[i]; - if ( v && v->IsMutableVal() ) - v->AsMutableVal()->RemoveProperties(RecursiveProps(arg_props)); - } - return true; - } - unsigned int RecordVal::MemoryAllocation() const { unsigned int size = 0; @@ -2599,37 +2391,6 @@ unsigned int VectorVal::ResizeAtLeast(unsigned int new_num_elements) return Resize(new_num_elements); } -bool VectorVal::AddProperties(Properties arg_props) - { - if ( ! MutableVal::AddProperties(arg_props) ) - return false; - - if ( ! RecursiveProps(arg_props) ) - return true; - - for ( unsigned int i = 0; i < val.vector_val->size(); ++i ) - if ( (*val.vector_val)[i]->IsMutableVal() ) - (*val.vector_val)[i]->AsMutableVal()->AddProperties(RecursiveProps(arg_props)); - - return true; - } - -bool VectorVal::RemoveProperties(Properties arg_props) - { - if ( ! MutableVal::RemoveProperties(arg_props) ) - return false; - - if ( ! RecursiveProps(arg_props) ) - return true; - - for ( unsigned int i = 0; i < val.vector_val->size(); ++i ) - if ( (*val.vector_val)[i]->IsMutableVal() ) - (*val.vector_val)[i]->AsMutableVal()->RemoveProperties(RecursiveProps(arg_props)); - - return true; - } - - Val* VectorVal::DoClone(CloneState* state) { auto vv = new VectorVal(vector_type); diff --git a/src/Val.h b/src/Val.h index 08038a686b..48f456fa17 100644 --- a/src/Val.h +++ b/src/Val.h @@ -325,20 +325,6 @@ public: return IsMutable(type->Tag()); } - const MutableVal* AsMutableVal() const - { - if ( ! IsMutableVal() ) - BadTag("Val::AsMutableVal", type_name(type->Tag())); - return (MutableVal*) this; - } - - MutableVal* AsMutableVal() - { - if ( ! IsMutableVal() ) - BadTag("Val::AsMutableVal", type_name(type->Tag())); - return (MutableVal*) this; - } - void Describe(ODesc* d) const override; virtual void DescribeReST(ODesc* d) const; @@ -499,56 +485,13 @@ private: extern ValManager* val_mgr; class MutableVal : public Val { -public: - // Each MutableVal gets a globally unique ID that can be used to - // reference it no matter if it's directly bound to any user-visible - // ID. This ID is inserted into the global namespace. - ID* UniqueID() const { return id ? id : Bind(); } - - // Returns true if we've already generated a unique ID. - bool HasUniqueID() const { return id; } - - // Transfers the unique ID of the given value to this value. We keep our - // old ID as an alias. - void TransferUniqueID(MutableVal* mv); - - // MutableVals can have properties (let's refrain from calling them - // attributes!). Most properties are recursive. If a derived object - // can contain MutableVals itself, the object has to override - // {Add,Remove}Properties(). RecursiveProp(state) masks out all non- - // recursive properties. If this is non-null, an overriden method must - // call itself with RecursiveProp(state) as argument for all contained - // values. (In any case, don't forget to call the parent's method.) - typedef char Properties; - - // Tracked by NotifierRegistry, not recursive. - static const int TRACKED = 0x04; - - int RecursiveProps(int prop) const { return prop & ~TRACKED; } - - Properties GetProperties() const { return props; } - virtual bool AddProperties(Properties state); - virtual bool RemoveProperties(Properties state); - protected: - explicit MutableVal(BroType* t) : Val(t) - { props = 0; id = 0; } - MutableVal() { props = 0; id = 0; } + explicit MutableVal(BroType* t) : Val(t) {} + + MutableVal() {} ~MutableVal() override; - friend class ID; - friend class Val; - - void SetID(ID* arg_id) { Unref(id); id = arg_id; } void Modified() { notifiers.Modified(this); } - -private: - ID* Bind() const; - - mutable ID* id; - list aliases; - Properties props; - uint64 last_modified; }; #define Microseconds 1e-6 @@ -771,7 +714,7 @@ public: { val = v; last_access_time = network_time; - expire_access_time = last_read_update = + expire_access_time = int(network_time - bro_start_network_time); } @@ -780,7 +723,6 @@ public: auto rval = new TableEntryVal(val ? val->Clone() : nullptr); rval->last_access_time = last_access_time; rval->expire_access_time = expire_access_time; - rval->last_read_update = last_read_update; return rval; } @@ -796,24 +738,16 @@ public: void SetExpireAccess(double time) { expire_access_time = int(time - bro_start_network_time); } - // Returns/sets time of when we propagated the last OP_READ_IDX - // for this item. - double LastReadUpdate() const - { return bro_start_network_time + last_read_update; } - void SetLastReadUpdate(double time) - { last_read_update = int(time - bro_start_network_time); } - protected: friend class TableVal; Val* val; double last_access_time; - // The next two entries store seconds since Bro's start. We use - // ints here to save a few bytes, as we do not need a high resolution - // for these anyway. + // The next entry stores seconds since Bro's start. We use ints here + // to save a few bytes, as we do not need a high resolution for these + // anyway. int expire_access_time; - int last_read_update; }; class TableValTimer : public Timer { @@ -953,9 +887,6 @@ protected: int ExpandCompoundAndInit(val_list* vl, int k, Val* new_val); int CheckAndAssign(Val* index, Val* new_val); - bool AddProperties(Properties arg_state) override; - bool RemoveProperties(Properties arg_state) override; - // Calculates default value for index. Returns 0 if none. Val* Default(Val* index); @@ -971,9 +902,6 @@ protected: // takes ownership of the reference. double CallExpireFunc(Val *idx); - // Propagates a read operation if necessary. - void ReadOperation(Val* index, TableEntryVal *v); - Val* DoClone(CloneState* state) override; TableType* table_type; @@ -1043,9 +971,6 @@ protected: friend class Val; RecordVal() {} - bool AddProperties(Properties arg_state) override; - bool RemoveProperties(Properties arg_state) override; - Val* DoClone(CloneState* state) override; RecordType* record_type; @@ -1133,8 +1058,6 @@ protected: friend class Val; VectorVal() { } - bool AddProperties(Properties arg_state) override; - bool RemoveProperties(Properties arg_state) override; void ValDescribe(ODesc* d) const override; Val* DoClone(CloneState* state) override; From 062a1ee6b37b7686b7eea7c455901797316a937d Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Thu, 6 Jun 2019 23:13:43 +0000 Subject: [PATCH 07/10] Redo API for notifiers. There's now an notifier::Modifiable interface class that class supposed to signal modifications are to be derived from. This takes the place of the former MutableValue class and also unifies how Val and IDs signal modifications. --- src/ID.cc | 2 +- src/ID.h | 2 +- src/StateAccess.cc | 99 +++++++++-------------------------- src/StateAccess.h | 125 ++++++++++++++++++++++++++------------------- src/Trigger.cc | 30 ++++------- src/Trigger.h | 9 ++-- src/Val.cc | 6 +-- src/Val.h | 16 ++++-- 8 files changed, 126 insertions(+), 163 deletions(-) diff --git a/src/ID.cc b/src/ID.cc index 582c8fcf8e..894de949a5 100644 --- a/src/ID.cc +++ b/src/ID.cc @@ -66,7 +66,7 @@ void ID::SetVal(Val* v, bool arg_weak_ref) val = v; weak_ref = arg_weak_ref; - notifiers.Modified(this); + Modified(); #ifdef DEBUG UpdateValID(); diff --git a/src/ID.h b/src/ID.h index 7717a584f3..a1cbe494d3 100644 --- a/src/ID.h +++ b/src/ID.h @@ -15,7 +15,7 @@ class Func; typedef enum { INIT_NONE, INIT_FULL, INIT_EXTRA, INIT_REMOVE, } init_class; typedef enum { SCOPE_FUNCTION, SCOPE_MODULE, SCOPE_GLOBAL } IDScope; -class ID : public BroObj { +class ID : public BroObj, public notifier::Modifiable { public: ID(const char* name, IDScope arg_scope, bool arg_is_export); ~ID() override; diff --git a/src/StateAccess.cc b/src/StateAccess.cc index 853a8a7879..7d7c397981 100644 --- a/src/StateAccess.cc +++ b/src/StateAccess.cc @@ -4,110 +4,57 @@ #include "NetVar.h" #include "DebugLogger.h" -NotifierRegistry notifiers; +notifier::Registry notifier::registry; -NotifierRegistry::~NotifierRegistry() +notifier::Registry::~Registry() { - for ( auto i : ids ) - Unref(i.first); - - for ( auto i : vals ) - Unref(i.first); + for ( auto i : registrations ) + Unregister(i.first); } -void NotifierRegistry::Register(ID* id, NotifierRegistry::Notifier* notifier) +void notifier::Registry::Register(Modifiable* m, notifier::Notifier* notifier) { - DBG_LOG(DBG_NOTIFIERS, "registering ID %s for notifier %s", - id->Name(), notifier->Name()); + DBG_LOG(DBG_NOTIFIERS, "registering modifiable %p for notifier %s", + m, notifier->Name()); - Attr* attr = new Attr(ATTR_TRACKED); - - if ( id->Attrs() ) - { - if ( ! id->Attrs()->FindAttr(ATTR_TRACKED) ) - id->Attrs()->AddAttr(attr); - } - else - { - attr_list* a = new attr_list{attr}; - id->SetAttrs(new Attributes(a, id->Type(), false)); - } - - Unref(attr); - - ids.insert({id, notifier}); - Ref(id); + registrations.insert({m, notifier}); + ++m->notifiers; } -void NotifierRegistry::Register(Val* val, NotifierRegistry::Notifier* notifier) +void notifier::Registry::Unregister(Modifiable* m, notifier::Notifier* notifier) { - if ( ! val->IsMutableVal() ) - return; + DBG_LOG(DBG_NOTIFIERS, "unregistering modifiable %p from notifier %s", + m, notifier->Name()); - DBG_LOG(DBG_NOTIFIERS, "registering value %p for notifier %s", - val, notifier->Name()); - - vals.insert({val, notifier}); - Ref(val); - } - -void NotifierRegistry::Unregister(ID* id, NotifierRegistry::Notifier* notifier) - { - DBG_LOG(DBG_NOTIFIERS, "unregistering ID %s for notifier %s", - id->Name(), notifier->Name()); - - auto x = ids.equal_range(id); + auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) { if ( i->second == notifier ) { - ids.erase(i); - Unref(id); - break; - } - } - - if ( ids.find(id) == ids.end() ) - // Last registered notifier for this ID - id->Attrs()->RemoveAttr(ATTR_TRACKED); - } - -void NotifierRegistry::Unregister(Val* val, NotifierRegistry::Notifier* notifier) - { - DBG_LOG(DBG_NOTIFIERS, "unregistering Val %p for notifier %s", - val, notifier->Name()); - - auto x = vals.equal_range(val); - for ( auto i = x.first; i != x.second; i++ ) - { - if ( i->second == notifier ) - { - vals.erase(i); - Unref(val); + registrations.erase(i); + --i->first->notifiers; break; } } } -void NotifierRegistry::Modified(Val *val) +void notifier::Registry::Unregister(Modifiable* m) { - DBG_LOG(DBG_NOTIFIERS, "modification to tracked value %p", val); - - auto x = vals.equal_range(val); + auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) - i->second->Modified(val); + Unregister(m, i->second); } -void NotifierRegistry::Modified(ID *id) +void notifier::Registry::Modified(Modifiable* m) { - DBG_LOG(DBG_NOTIFIERS, "modification to tracked ID %s", id->Name()); + DBG_LOG(DBG_NOTIFIERS, "modification to modifiable %p", m); - auto x = ids.equal_range(id); + auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) - i->second->Modified(id); + i->second->Modified(m); } -const char* NotifierRegistry::Notifier::Name() const +const char* notifier::Notifier::Name() const { return fmt("%p", this); } diff --git a/src/StateAccess.h b/src/StateAccess.h index 528e3357d5..91a7fa6186 100644 --- a/src/StateAccess.h +++ b/src/StateAccess.h @@ -1,4 +1,14 @@ // A class describing a state-modyfing access to a Value or an ID. +// +// TODO UPDATE: We provide a notifier framework to inform interested parties +// of modifications to selected global IDs/Vals. To get notified about a +// change, derive a class from Notifier and register the interesting +// instances with the NotifierRegistry. +// +// Note: For containers (e.g., tables), notifications are only issued if the +// container itself is modified, *not* for changes to the values contained +// therein. + #ifndef STATEACESSS_H #define STATEACESSS_H @@ -7,63 +17,74 @@ #include #include -class Val; -class ID; -class MutableVal; -class HashKey; -class ODesc; -class TableVal; +#include "util.h" -// We provide a notifier framework to inform interested parties of -// modifications to selected global IDs/Vals. To get notified about a change, -// derive a class from Notifier and register the interesting IDs/Vals with -// the NotifierRegistry. -// -// Note: For containers (e.g., tables), notifications are only issued if the -// container itself is modified, *not* for changes to the values contained -// therein. +namespace notifier { -class NotifierRegistry { +class Modifiable; + +class Notifier { public: - class Notifier { - public: - virtual ~Notifier() { } + virtual ~Notifier() { } - // Called when a change is being performed. Note that when - // these methods are called, it is undefined whether the - // change has already been done or is just going to be - // performed soon. - virtual void Modified(ID* id) = 0; - virtual void Modified(Val* val) = 0; - virtual const char* Name() const; // for debugging - }; - - NotifierRegistry() { } - ~NotifierRegistry(); - - // Register a new notifier to be informed when ID/Val changes. Note - // that the registry will store a reference to the target, keeping - // the instance alive for as long as it's registered. - void Register(ID* id, Notifier* notifier); - void Register(Val* val, Notifier* notifier); - - // Cancel a notifier's tracking for this ID/Val, also releasing the - // referencee being held. - void Unregister(ID* id, Notifier* notifier); - void Unregister(Val* val, Notifier* notifier); - - // Inform all registered notifiiers of a modification to a value/ID. - void Modified(ID *id); - void Modified(Val *val); - -private: - typedef std::unordered_multimap ValMap; - typedef std::unordered_multimap IDMap; - - ValMap vals; - IDMap ids; + // Called afger a change has been performed. + virtual void Modified(Modifiable* m) = 0; + virtual const char* Name() const; // for debugging }; -extern NotifierRegistry notifiers; +// Singleton class. +class Registry { +public: + Registry() { } + ~Registry(); + + // Register a new notifier to be informed when an instance changes. + void Register(Modifiable* m, Notifier* notifier); + + // Cancel a notifier's tracking an instace. + void Unregister(Modifiable* m, Notifier* notifier); + + // Cancel all notifiers registered for an instance. + void Unregister(Modifiable* m); + +private: + friend class Modifiable; + + // Inform all registered notifiers of a modification to an instance. + void Modified(Modifiable* m); + + typedef std::unordered_multimap ModifiableMap; + ModifiableMap registrations; +}; + + +class Registry; +extern Registry registry; + +// Base class for objects wanting to signal modifications to the registry. +class Modifiable { +protected: + friend class Registry; + + Modifiable() {} + ~Modifiable() + { + if ( notifiers ) + registry.Unregister(this); + + }; + + void Modified() + { + if ( notifiers ) + registry.Modified(this); + } + + // Number of currently registered notifiers for this instance. + uint64 notifiers; +}; + +} + #endif diff --git a/src/Trigger.cc b/src/Trigger.cc index 213707b6b8..c1295b8218 100644 --- a/src/Trigger.cc +++ b/src/Trigger.cc @@ -33,7 +33,7 @@ TraversalCode TriggerTraversalCallback::PreExpr(const Expr* expr) trigger->Register(e->Id()); Val* v = e->Id()->ID_Val(); - if ( v && v->IsMutableVal() ) + if ( v && v->Modifiable() ) trigger->Register(v); break; }; @@ -382,38 +382,30 @@ void Trigger::Timeout() void Trigger::Register(ID* id) { assert(! disabled); - notifiers.Register(id, this); + notifier::registry.Register(id, this); Ref(id); - ids.insert(id); + objs.push_back(id); } void Trigger::Register(Val* val) { + if ( ! val->Modifiable() ) + return; + assert(! disabled); - notifiers.Register(val, this); + notifier::registry.Register(val->Modifiable(), this); Ref(val); - vals.insert(val); + objs.push_back(val); } void Trigger::UnregisterAll() { - loop_over_list(ids, i) - { - notifiers.Unregister(ids[i], this); - Unref(ids[i]); - } + for ( auto o : objs ) + Unref(o); // this will unregister with the registry as well - ids.clear(); - - loop_over_list(vals, j) - { - notifiers.Unregister(vals[j], this); - Unref(vals[j]); - } - - vals.clear(); + objs.clear(); } void Trigger::Attach(Trigger *trigger) diff --git a/src/Trigger.h b/src/Trigger.h index 8dc478e049..fe7d425a72 100644 --- a/src/Trigger.h +++ b/src/Trigger.h @@ -13,7 +13,7 @@ class TriggerTimer; class TriggerTraversalCallback; -class Trigger : public NotifierRegistry::Notifier, public BroObj { +class Trigger : public notifier::Notifier, public BroObj { public: // Don't access Trigger objects; they take care of themselves after // instantiation. Note that if the condition is already true, the @@ -61,9 +61,7 @@ public: { d->Add(""); } // Overidden from Notifier. We queue the trigger and evaluate it // later to avoid race conditions. - void Modified(ID* id) override - { QueueTrigger(this); } - void Modified(Val* val) override + void Modified(notifier::Modifiable* m) override { QueueTrigger(this); } const char* Name() const override; @@ -104,8 +102,7 @@ private: bool delayed; // true if a function call is currently being delayed bool disabled; - val_list vals; - id_list ids; + std::vector objs; typedef map ValCache; ValCache cache; diff --git a/src/Val.cc b/src/Val.cc index 4ba2ea6dd7..bea9ce4038 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -899,7 +899,7 @@ static void table_entry_val_delete_func(void* val) delete tv; } -TableVal::TableVal(TableType* t, Attributes* a) : MutableVal(t) +TableVal::TableVal(TableType* t, Attributes* a) : Val(t) { Init(t); SetAttrs(a); @@ -1982,7 +1982,7 @@ unsigned int TableVal::MemoryAllocation() const vector RecordVal::parse_time_records; -RecordVal::RecordVal(RecordType* t, bool init_fields) : MutableVal(t) +RecordVal::RecordVal(RecordType* t, bool init_fields) : Val(t) { origin = 0; record_type = t; @@ -2287,7 +2287,7 @@ Val* EnumVal::DoClone(CloneState* state) return Ref(); } -VectorVal::VectorVal(VectorType* t) : MutableVal(t) +VectorVal::VectorVal(VectorType* t) : Val(t) { vector_type = t->Ref()->AsVectorType(); val.vector_val = new vector(); diff --git a/src/Val.h b/src/Val.h index 48f456fa17..d98c352f6d 100644 --- a/src/Val.h +++ b/src/Val.h @@ -328,6 +328,8 @@ public: void Describe(ODesc* d) const override; virtual void DescribeReST(ODesc* d) const; + virtual notifier::Modifiable* Modifiable() { return 0; } + #ifdef DEBUG // For debugging, we keep a reference to the global ID to which a // value has been bound *last*. @@ -490,8 +492,6 @@ protected: MutableVal() {} ~MutableVal() override; - - void Modified() { notifiers.Modified(this); } }; #define Microseconds 1e-6 @@ -764,7 +764,7 @@ protected: }; class CompositeHash; -class TableVal : public MutableVal { +class TableVal : public Val, public notifier::Modifiable { public: explicit TableVal(TableType* t, Attributes* attrs = 0); ~TableVal() override; @@ -877,6 +877,8 @@ public: HashKey* ComputeHash(const Val* index) const { return table_hash->ComputeHash(index, 1); } + notifier::Modifiable* Modifiable() override { return this; } + protected: friend class Val; TableVal() {} @@ -915,7 +917,7 @@ protected: Val* def_val; }; -class RecordVal : public MutableVal { +class RecordVal : public Val, public notifier::Modifiable { public: explicit RecordVal(RecordType* t, bool init_fields = true); ~RecordVal() override; @@ -962,6 +964,8 @@ public: unsigned int MemoryAllocation() const override; void DescribeReST(ODesc* d) const override; + notifier::Modifiable* Modifiable() override { return this; } + // Extend the underlying arrays of record instances created during // parsing to match the number of fields in the record type (they may // mismatch as a result of parse-time record type redefinitions. @@ -1006,7 +1010,7 @@ protected: }; -class VectorVal : public MutableVal { +class VectorVal : public Val, public notifier::Modifiable { public: explicit VectorVal(VectorType* t); ~VectorVal() override; @@ -1054,6 +1058,8 @@ public: // Won't shrink size. unsigned int ResizeAtLeast(unsigned int new_num_elements); + notifier::Modifiable* Modifiable() override { return this; } + protected: friend class Val; VectorVal() { } From 7bd738865cdab1d5983a96a16f5d199166cb3fcb Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Thu, 6 Jun 2019 23:48:31 +0000 Subject: [PATCH 08/10] Remove MutableVal class. --- src/ID.h | 4 ---- src/StateAccess.cc | 23 ++++++++++++++--------- src/StateAccess.h | 21 ++++++++++++--------- src/Stats.cc | 2 +- src/Trigger.cc | 11 ++++++++--- src/Trigger.h | 4 ++-- src/Type.h | 4 ---- src/Val.cc | 4 ---- src/Val.h | 14 -------------- src/bro.bif | 5 +---- src/input/readers/config/Config.cc | 2 +- 11 files changed, 39 insertions(+), 55 deletions(-) diff --git a/src/ID.h b/src/ID.h index a1cbe494d3..9c9a842c39 100644 --- a/src/ID.h +++ b/src/ID.h @@ -70,10 +70,6 @@ public: bool IsRedefinable() const { return FindAttr(ATTR_REDEF) != 0; } - // Returns true if ID is one of those internal globally unique IDs - // to which MutableVals are bound (there name start with a '#'). - bool IsInternalGlobal() const { return name && name[0] == '#'; } - void SetAttrs(Attributes* attr); void AddAttrs(Attributes* attr); void RemoveAttr(attr_tag a); diff --git a/src/StateAccess.cc b/src/StateAccess.cc index 7d7c397981..b580215271 100644 --- a/src/StateAccess.cc +++ b/src/StateAccess.cc @@ -14,8 +14,8 @@ notifier::Registry::~Registry() void notifier::Registry::Register(Modifiable* m, notifier::Notifier* notifier) { - DBG_LOG(DBG_NOTIFIERS, "registering modifiable %p for notifier %s", - m, notifier->Name()); + DBG_LOG(DBG_NOTIFIERS, "registering modifiable %p for notifier %p", + m, notifier); registrations.insert({m, notifier}); ++m->notifiers; @@ -23,16 +23,16 @@ void notifier::Registry::Register(Modifiable* m, notifier::Notifier* notifier) void notifier::Registry::Unregister(Modifiable* m, notifier::Notifier* notifier) { - DBG_LOG(DBG_NOTIFIERS, "unregistering modifiable %p from notifier %s", - m, notifier->Name()); + DBG_LOG(DBG_NOTIFIERS, "unregistering modifiable %p from notifier %p", + m, notifier); auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) { if ( i->second == notifier ) { - registrations.erase(i); --i->first->notifiers; + registrations.erase(i); break; } } @@ -40,9 +40,14 @@ void notifier::Registry::Unregister(Modifiable* m, notifier::Notifier* notifier) void notifier::Registry::Unregister(Modifiable* m) { + DBG_LOG(DBG_NOTIFIERS, "unregistering modifiable %p from all notifiers", + m); + auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) - Unregister(m, i->second); + --i->first->notifiers; + + registrations.erase(x.first, x.second); } void notifier::Registry::Modified(Modifiable* m) @@ -54,8 +59,8 @@ void notifier::Registry::Modified(Modifiable* m) i->second->Modified(m); } -const char* notifier::Notifier::Name() const +notifier::Modifiable::~Modifiable() { - return fmt("%p", this); + if ( notifiers ) + registry.Unregister(this); } - diff --git a/src/StateAccess.h b/src/StateAccess.h index 91a7fa6186..b769e320c0 100644 --- a/src/StateAccess.h +++ b/src/StateAccess.h @@ -18,6 +18,7 @@ #include #include "util.h" +#include "DebugLogger.h" namespace notifier { @@ -25,11 +26,18 @@ class Modifiable; class Notifier { public: - virtual ~Notifier() { } + Notifier() + { + DBG_LOG(DBG_NOTIFIERS, "creating notifier %p", this); + } - // Called afger a change has been performed. + virtual ~Notifier() + { + DBG_LOG(DBG_NOTIFIERS, "destroying notifier %p", this); + } + + // Called after a change has been performed. virtual void Modified(Modifiable* m) = 0; - virtual const char* Name() const; // for debugging }; // Singleton class. @@ -67,12 +75,7 @@ protected: friend class Registry; Modifiable() {} - ~Modifiable() - { - if ( notifiers ) - registry.Unregister(this); - - }; + virtual ~Modifiable(); void Modified() { diff --git a/src/Stats.cc b/src/Stats.cc index 1d2a2c8ad8..9489f12f93 100644 --- a/src/Stats.cc +++ b/src/Stats.cc @@ -255,7 +255,7 @@ void ProfileLogger::Log() while ( (id = globals->NextEntry(c)) ) // We don't show/count internal globals as they are always // contained in some other global user-visible container. - if ( id->HasVal() && ! id->IsInternalGlobal() ) + if ( id->HasVal() ) { Val* v = id->ID_Val(); diff --git a/src/Trigger.cc b/src/Trigger.cc index c1295b8218..8de5dff5bd 100644 --- a/src/Trigger.cc +++ b/src/Trigger.cc @@ -385,7 +385,7 @@ void Trigger::Register(ID* id) notifier::registry.Register(id, this); Ref(id); - objs.push_back(id); + objs.push_back({id, id}); } void Trigger::Register(Val* val) @@ -397,13 +397,18 @@ void Trigger::Register(Val* val) notifier::registry.Register(val->Modifiable(), this); Ref(val); - objs.push_back(val); + objs.emplace_back(val, val->Modifiable()); } void Trigger::UnregisterAll() { + DBG_LOG(DBG_NOTIFIERS, "%s: unregistering all", Name()); + for ( auto o : objs ) - Unref(o); // this will unregister with the registry as well + { + notifier::registry.Unregister(o.second, this); + Unref(o.first); + } objs.clear(); } diff --git a/src/Trigger.h b/src/Trigger.h index fe7d425a72..3004d30732 100644 --- a/src/Trigger.h +++ b/src/Trigger.h @@ -64,7 +64,7 @@ public: void Modified(notifier::Modifiable* m) override { QueueTrigger(this); } - const char* Name() const override; + const char* Name() const; static void QueueTrigger(Trigger* trigger); @@ -102,7 +102,7 @@ private: bool delayed; // true if a function call is currently being delayed bool disabled; - std::vector objs; + std::vector> objs; typedef map ValCache; ValCache cache; diff --git a/src/Type.h b/src/Type.h index a97f7360c8..bc8d673443 100644 --- a/src/Type.h +++ b/src/Type.h @@ -693,10 +693,6 @@ bool is_atomic_type(const BroType* t); // True if the given type tag corresponds to a function type. #define IsFunc(t) (t == TYPE_FUNC) -// True if the given type tag corresponds to mutable type. -#define IsMutable(t) \ - (t == TYPE_RECORD || t == TYPE_TABLE || t == TYPE_VECTOR) - // True if the given type type is a vector. #define IsVector(t) (t == TYPE_VECTOR) diff --git a/src/Val.cc b/src/Val.cc index bea9ce4038..0d6f5838ca 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -349,10 +349,6 @@ void Val::ValDescribeReST(ODesc* d) const } } -MutableVal::~MutableVal() - { - } - IntervalVal::IntervalVal(double quantity, double units) : Val(quantity * units, TYPE_INTERVAL) { diff --git a/src/Val.h b/src/Val.h index d98c352f6d..48bbd50cfc 100644 --- a/src/Val.h +++ b/src/Val.h @@ -49,7 +49,6 @@ class RecordVal; class ListVal; class StringVal; class EnumVal; -class MutableVal; class VectorVal; @@ -320,11 +319,6 @@ public: CONST_CONVERTER(TYPE_STRING, StringVal*, AsStringVal) CONST_CONVERTER(TYPE_VECTOR, VectorVal*, AsVectorVal) - bool IsMutableVal() const - { - return IsMutable(type->Tag()); - } - void Describe(ODesc* d) const override; virtual void DescribeReST(ODesc* d) const; @@ -486,14 +480,6 @@ private: extern ValManager* val_mgr; -class MutableVal : public Val { -protected: - explicit MutableVal(BroType* t) : Val(t) {} - - MutableVal() {} - ~MutableVal() override; -}; - #define Microseconds 1e-6 #define Milliseconds 1e-3 #define Seconds 1.0 diff --git a/src/bro.bif b/src/bro.bif index 47b713e4e1..e143cf16e8 100644 --- a/src/bro.bif +++ b/src/bro.bif @@ -1819,7 +1819,7 @@ function global_sizes%(%): var_sizes ID* id; while ( (id = globals->NextEntry(c)) ) - if ( id->HasVal() && ! id->IsInternalGlobal() ) + if ( id->HasVal() ) { Val* id_name = new StringVal(id->Name()); Val* id_size = val_mgr->GetCount(id->ID_Val()->MemoryAllocation()); @@ -1847,9 +1847,6 @@ function global_ids%(%): id_table ID* id; while ( (id = globals->NextEntry(c)) ) { - if ( id->IsInternalGlobal() ) - continue; - RecordVal* rec = new RecordVal(script_id); rec->Assign(0, new StringVal(type_name(id->Type()->Tag()))); rec->Assign(1, val_mgr->GetBool(id->IsExport())); diff --git a/src/input/readers/config/Config.cc b/src/input/readers/config/Config.cc index eca276281c..7c1708babd 100644 --- a/src/input/readers/config/Config.cc +++ b/src/input/readers/config/Config.cc @@ -33,7 +33,7 @@ Config::Config(ReaderFrontend *frontend) : ReaderBackend(frontend) while ( auto id = globals->NextEntry(c) ) { - if ( id->IsInternalGlobal() || ! id->IsOption() ) + if ( ! id->IsOption() ) continue; if ( id->Type()->Tag() == TYPE_RECORD || From 6adab8d46a98db691fbc1ed3ae4933d519183b1e Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 7 Jun 2019 23:38:25 +0000 Subject: [PATCH 09/10] Clean up new code. --- src/StateAccess.cc | 42 +++++++++-------- src/StateAccess.h | 109 +++++++++++++++++++++++++++------------------ src/Trigger.cc | 2 +- src/Trigger.h | 2 +- src/Val.h | 2 + 5 files changed, 94 insertions(+), 63 deletions(-) diff --git a/src/StateAccess.cc b/src/StateAccess.cc index b580215271..dcf5ef8b21 100644 --- a/src/StateAccess.cc +++ b/src/StateAccess.cc @@ -1,37 +1,44 @@ -#include "Val.h" +// See the file "COPYING" in the main distribution directory for copyright. + #include "StateAccess.h" -#include "Event.h" -#include "NetVar.h" #include "DebugLogger.h" notifier::Registry notifier::registry; +notifier::Receiver::Receiver() + { + DBG_LOG(DBG_NOTIFIERS, "creating receiver %p", this); + } + +notifier::Receiver::~Receiver() + { + DBG_LOG(DBG_NOTIFIERS, "deleting receiver %p", this); + } + notifier::Registry::~Registry() { for ( auto i : registrations ) Unregister(i.first); } -void notifier::Registry::Register(Modifiable* m, notifier::Notifier* notifier) +void notifier::Registry::Register(Modifiable* m, notifier::Receiver* r) { - DBG_LOG(DBG_NOTIFIERS, "registering modifiable %p for notifier %p", - m, notifier); + DBG_LOG(DBG_NOTIFIERS, "registering object %p for receiver %p", m, r); - registrations.insert({m, notifier}); - ++m->notifiers; + registrations.insert({m, r}); + ++m->num_receivers; } -void notifier::Registry::Unregister(Modifiable* m, notifier::Notifier* notifier) +void notifier::Registry::Unregister(Modifiable* m, notifier::Receiver* r) { - DBG_LOG(DBG_NOTIFIERS, "unregistering modifiable %p from notifier %p", - m, notifier); + DBG_LOG(DBG_NOTIFIERS, "unregistering object %p from receiver %p", m, r); auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) { - if ( i->second == notifier ) + if ( i->second == r ) { - --i->first->notifiers; + --i->first->num_receivers; registrations.erase(i); break; } @@ -40,19 +47,18 @@ void notifier::Registry::Unregister(Modifiable* m, notifier::Notifier* notifier) void notifier::Registry::Unregister(Modifiable* m) { - DBG_LOG(DBG_NOTIFIERS, "unregistering modifiable %p from all notifiers", - m); + DBG_LOG(DBG_NOTIFIERS, "unregistering object %p from all notifiers", m); auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) - --i->first->notifiers; + --i->first->num_receivers; registrations.erase(x.first, x.second); } void notifier::Registry::Modified(Modifiable* m) { - DBG_LOG(DBG_NOTIFIERS, "modification to modifiable %p", m); + DBG_LOG(DBG_NOTIFIERS, "object %p has been modified", m); auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) @@ -61,6 +67,6 @@ void notifier::Registry::Modified(Modifiable* m) notifier::Modifiable::~Modifiable() { - if ( notifiers ) + if ( num_receivers ) registry.Unregister(this); } diff --git a/src/StateAccess.h b/src/StateAccess.h index b769e320c0..2361801ec4 100644 --- a/src/StateAccess.h +++ b/src/StateAccess.h @@ -1,14 +1,9 @@ -// A class describing a state-modyfing access to a Value or an ID. +// See the file "COPYING" in the main distribution directory for copyright. // -// TODO UPDATE: We provide a notifier framework to inform interested parties -// of modifications to selected global IDs/Vals. To get notified about a -// change, derive a class from Notifier and register the interesting -// instances with the NotifierRegistry. -// -// Note: For containers (e.g., tables), notifications are only issued if the -// container itself is modified, *not* for changes to the values contained -// therein. - +// A notification framework to inform interested parties of modifications to +// selected global objects. To get notified about a change, derive a class +// from notifier::Receiver and register the interesting objects with the +// notification::Registry. #ifndef STATEACESSS_H #define STATEACESSS_H @@ -24,70 +19,98 @@ namespace notifier { class Modifiable; -class Notifier { +/** Interface class for receivers of notifications. */ +class Receiver { public: - Notifier() - { - DBG_LOG(DBG_NOTIFIERS, "creating notifier %p", this); - } + Receiver(); + virtual ~Receiver(); - virtual ~Notifier() - { - DBG_LOG(DBG_NOTIFIERS, "destroying notifier %p", this); - } - - // Called after a change has been performed. + /** + * Callback executed when a register object has been modified. + * + * @param m object that was modified + */ virtual void Modified(Modifiable* m) = 0; }; -// Singleton class. +/** Singleton class tracking all notification requests globally. */ class Registry { public: - Registry() { } ~Registry(); - // Register a new notifier to be informed when an instance changes. - void Register(Modifiable* m, Notifier* notifier); + /** + * Registers a receiver to be informed when a modifiable object has + * changed. + * + * @param m object to track. Does not take ownership, but the object + * will automatically unregister itself on destruction. + * + * @param r receiver to notify on changes. Does not take ownershop, + * the receiver must remain valid as long as the registration stays + * in place. + */ + void Register(Modifiable* m, Receiver* r); - // Cancel a notifier's tracking an instace. - void Unregister(Modifiable* m, Notifier* notifier); + /** + * Cancels a receiver's request to be informed about an object's + * modification. The arguments to the method must match what was + * originally registered. + * + * @param m object to no loger track. + * + * @param r receiver to no longer notify. + */ + void Unregister(Modifiable* m, Receiver* Receiver); - // Cancel all notifiers registered for an instance. + /** + * Cancels any active receiver requests to be informed about a + * partilar object's modifications. + * + * @param m object to no loger track. + */ void Unregister(Modifiable* m); private: friend class Modifiable; - // Inform all registered notifiers of a modification to an instance. + // Inform all registered receivers of a modification to an object. + // Will be called from the object itself. void Modified(Modifiable* m); - typedef std::unordered_multimap ModifiableMap; + typedef std::unordered_multimap ModifiableMap; ModifiableMap registrations; }; - -class Registry; +/** + * Singleton object tracking all global notification requests. + */ extern Registry registry; -// Base class for objects wanting to signal modifications to the registry. +/** + * Base class for objects that can trigger notifications to receivers when + * modified. + */ class Modifiable { -protected: - friend class Registry; - - Modifiable() {} - virtual ~Modifiable(); - +public: + /** + * Calling this method signals to all registered receivers that the + * object has been modified. + */ void Modified() { - if ( notifiers ) + if ( num_receivers ) registry.Modified(this); } - // Number of currently registered notifiers for this instance. - uint64 notifiers; +protected: + friend class Registry; + + virtual ~Modifiable(); + + // Number of currently registered receivers. + uint64 num_receivers; }; } - #endif diff --git a/src/Trigger.cc b/src/Trigger.cc index 8de5dff5bd..ae6483e3f5 100644 --- a/src/Trigger.cc +++ b/src/Trigger.cc @@ -404,7 +404,7 @@ void Trigger::UnregisterAll() { DBG_LOG(DBG_NOTIFIERS, "%s: unregistering all", Name()); - for ( auto o : objs ) + for ( const auto& o : objs ) { notifier::registry.Unregister(o.second, this); Unref(o.first); diff --git a/src/Trigger.h b/src/Trigger.h index 3004d30732..0de5499045 100644 --- a/src/Trigger.h +++ b/src/Trigger.h @@ -13,7 +13,7 @@ class TriggerTimer; class TriggerTraversalCallback; -class Trigger : public notifier::Notifier, public BroObj { +class Trigger : public BroObj, public notifier::Receiver { public: // Don't access Trigger objects; they take care of themselves after // instantiation. Note that if the condition is already true, the diff --git a/src/Val.h b/src/Val.h index 48bbd50cfc..8eb33d1f0e 100644 --- a/src/Val.h +++ b/src/Val.h @@ -322,6 +322,8 @@ public: void Describe(ODesc* d) const override; virtual void DescribeReST(ODesc* d) const; + // To be overridden by mutable derived class to enable change + // notification. virtual notifier::Modifiable* Modifiable() { return 0; } #ifdef DEBUG From 32f30b5c716ccab9cd8a0d9eac6e8a63b473ad14 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Sat, 8 Jun 2019 00:27:23 +0000 Subject: [PATCH 10/10] Renaming src/StateAccess.{h,cc} to src/Notifier.{h,cc}. The old names did not reflect the content of the files anymore. --- src/CMakeLists.txt | 2 +- src/ID.h | 2 +- src/{StateAccess.cc => Notifier.cc} | 2 +- src/{StateAccess.h => Notifier.h} | 4 ++-- src/Trigger.h | 2 +- src/Val.h | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) rename src/{StateAccess.cc => Notifier.cc} (98%) rename src/{StateAccess.h => Notifier.h} (98%) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 1afa5193cc..6620779de8 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -302,7 +302,7 @@ set(bro_SRCS Scope.cc SerializationFormat.cc Sessions.cc - StateAccess.cc + Notifier.cc Stats.cc Stmt.cc Tag.cc diff --git a/src/ID.h b/src/ID.h index 9c9a842c39..ee60d4e61c 100644 --- a/src/ID.h +++ b/src/ID.h @@ -5,7 +5,7 @@ #include "Type.h" #include "Attr.h" -#include "StateAccess.h" +#include "Notifier.h" #include "TraverseTypes.h" #include diff --git a/src/StateAccess.cc b/src/Notifier.cc similarity index 98% rename from src/StateAccess.cc rename to src/Notifier.cc index dcf5ef8b21..15bc334af6 100644 --- a/src/StateAccess.cc +++ b/src/Notifier.cc @@ -1,7 +1,7 @@ // See the file "COPYING" in the main distribution directory for copyright. -#include "StateAccess.h" #include "DebugLogger.h" +#include "Notifier.h" notifier::Registry notifier::registry; diff --git a/src/StateAccess.h b/src/Notifier.h similarity index 98% rename from src/StateAccess.h rename to src/Notifier.h index 2361801ec4..0a399e526e 100644 --- a/src/StateAccess.h +++ b/src/Notifier.h @@ -5,8 +5,8 @@ // from notifier::Receiver and register the interesting objects with the // notification::Registry. -#ifndef STATEACESSS_H -#define STATEACESSS_H +#ifndef NOTIFIER_H +#define NOTIFIER_H #include #include diff --git a/src/Trigger.h b/src/Trigger.h index 0de5499045..2e0c91865f 100644 --- a/src/Trigger.h +++ b/src/Trigger.h @@ -4,7 +4,7 @@ #include #include -#include "StateAccess.h" +#include "Notifier.h" #include "Traverse.h" // Triggers are the heart of "when" statements: expressions that when diff --git a/src/Val.h b/src/Val.h index 8eb33d1f0e..959408da8c 100644 --- a/src/Val.h +++ b/src/Val.h @@ -18,7 +18,7 @@ #include "Timer.h" #include "ID.h" #include "Scope.h" -#include "StateAccess.h" +#include "Notifier.h" #include "IPAddr.h" #include "DebugLogger.h"