diff --git a/TODO.logging b/TODO.logging index f8208b79f8..a73f027750 100644 --- a/TODO.logging +++ b/TODO.logging @@ -1,21 +1,17 @@ List of the things not implemented yet: - - Cluster-style remote_print - - Rotation support - - Spawning writers in separate threads (not clear if we want that initially). + - Cluster-style remote_print. + - Rotation support. - Not sure if the logging does the right thing with &optional and &default values. Needs testing. - - Seems we could do some of the filter-related type checks - currently done dynamically at startup via a TraversalCallback. - - LogMgs' error handling - Put script function/constants into namespace. + - Some typchecks are still missing. - Cleanup code. - -There's probably more missing. + - Spawning writers in separate threads (not clear if we want that initially). Questions: - * Is giving the record column twice to create_stream too + * Is giving the record column twice to create_stream() too redundant? (once directly, and once via the event):: global ssh_log: event(rec: Log); diff --git a/src/LogMgr.cc b/src/LogMgr.cc index fc4b2bd02d..f6de4db2c1 100644 --- a/src/LogMgr.cc +++ b/src/LogMgr.cc @@ -89,6 +89,28 @@ LogMgr::Stream* LogMgr::FindStream(EnumVal* stream_id) return streams[idx]; } +void LogMgr::RemoveDisabledWriters(Stream* stream) + { + for ( list::iterator i = stream->filters.begin(); i != stream->filters.end(); ++i ) + { + Filter* filter = (*i); + + list disabled; + + for ( Filter::WriterMap::iterator j = filter->writers.begin(); j != filter->writers.end(); j++ ) + { + if ( j->second->Disabled() ) + { + delete j->second; + disabled.push_back(j->first); + } + } + + for ( list::iterator j = disabled.begin(); j != disabled.end(); j++ ) + filter->writers.erase(*j); + } + } + bool LogMgr::CreateStream(EnumVal* stream_id, RecordType* columns, EventHandlerPtr handler) { // TODO: Should check that the record has only supported types. @@ -272,7 +294,7 @@ bool LogMgr::AddFilter(EnumVal* stream_id, RecordVal* fval) // Get the path for the filter. Val* path_val = fval->Lookup(rtype->FieldOffset("path")); - if ( path_val ) + if ( path_val ) { filter->path = path_val->AsString()->CheckString(); filter->path_val = path_val->Ref(); @@ -349,6 +371,8 @@ bool LogMgr::RemoveFilter(EnumVal* stream_id, StringVal* filter) bool LogMgr::Write(EnumVal* stream_id, RecordVal* columns) { + bool error = false; + Stream* stream = FindStream(stream_id); if ( ! stream ) return false; @@ -387,7 +411,7 @@ bool LogMgr::Write(EnumVal* stream_id, RecordVal* columns) if ( ! result ) continue; } - + if ( filter->path_func ) { val_list vl(2); @@ -440,7 +464,8 @@ bool LogMgr::Write(EnumVal* stream_id, RecordVal* columns) // Alright, can do the write now. LogVal** vals = RecordToFilterVals(filter, columns); - writer->Write(vals); + if ( ! writer->Write(vals) ) + error = true; #ifdef DEBUG DBG_LOG(DBG_LOGGING, "Wrote record to filter '%s' on stream '%s'", filter->name.c_str(), stream->name.c_str()); @@ -448,6 +473,10 @@ bool LogMgr::Write(EnumVal* stream_id, RecordVal* columns) } Unref(columns); + + if ( error ) + RemoveDisabledWriters(stream); + return true; } @@ -545,9 +574,12 @@ bool LogMgr::SetBuf(EnumVal* stream_id, bool enabled) j->second->SetBuf(enabled); } + RemoveDisabledWriters(stream); + return true; } void LogMgr::Error(LogWriter* writer, const char* msg) { + run_time(fmt("error with writer for %s: %s", writer->Path().c_str(), msg)); } diff --git a/src/LogMgr.h b/src/LogMgr.h index 3357f871b5..bd269ccfde 100644 --- a/src/LogMgr.h +++ b/src/LogMgr.h @@ -76,6 +76,7 @@ private: bool TraverseRecord(Filter* filter, RecordType* rt, TableVal* include, TableVal* exclude, string path, list indices); LogVal** RecordToFilterVals(Filter* filter, RecordVal* columns); Stream* FindStream(EnumVal* stream_id); + void RemoveDisabledWriters(Stream* stream); vector streams; // Indexed by stream enum. }; diff --git a/src/LogWriter.cc b/src/LogWriter.cc index fd9bcf0b44..4292849a9a 100644 --- a/src/LogWriter.cc +++ b/src/LogWriter.cc @@ -7,6 +7,7 @@ LogWriter::LogWriter() buf = 0; buf_len = 1024; buffering = true; + disabled = false; } LogWriter::~LogWriter() @@ -22,7 +23,13 @@ bool LogWriter::Init(string arg_path, int arg_num_fields, LogField** arg_fields) path = arg_path; num_fields = arg_num_fields; fields = arg_fields; - DoInit(arg_path, arg_num_fields, arg_fields); + + if ( ! DoInit(arg_path, arg_num_fields, arg_fields) ) + { + disabled = true; + return false; + } + return true; } @@ -30,12 +37,11 @@ bool LogWriter::Write(LogVal** vals) { bool result = DoWrite(num_fields, fields, vals); DeleteVals(vals); - return result; - } -void LogWriter::Finish() - { - DoFinish(); + if ( ! result ) + disabled = true; + + return result; } bool LogWriter::SetBuf(bool enabled) @@ -45,7 +51,18 @@ bool LogWriter::SetBuf(bool enabled) return true; buffering = enabled; - return DoSetBuf(enabled); + if ( ! DoSetBuf(enabled) ) + { + disabled = true; + return false; + } + + return true; + } + +void LogWriter::Finish() + { + DoFinish(); } const char* LogWriter::Fmt(const char* format, ...) @@ -72,10 +89,9 @@ const char* LogWriter::Fmt(const char* format, ...) return buf; } - void LogWriter::Error(const char *msg) { - run_time(msg); + log_mgr->Error(this, msg); } void LogWriter::DeleteVals(LogVal** vals) diff --git a/src/LogWriter.h b/src/LogWriter.h index e13c06edff..780b8b2a4c 100644 --- a/src/LogWriter.h +++ b/src/LogWriter.h @@ -31,38 +31,38 @@ public: // occured, in which case the writer must not be used further. bool Write(LogVal** vals); + // Sets the buffering status for the writer, if the writer supports + bool SetBuf(bool enabled); + // Finished writing to this logger. Will not be called if an error has // been indicated earlier. After calling this, no more writing must be // performed. void Finish(); - // Sets the buffering status for the writer, if the writer supports - bool SetBuf(bool enabled); - protected: - //// Methods for Writers to override. + // Methods for Writers to override. If any of these returs false, it will + // be assumed that a fatal error has occured that prevents the writer + // from further operation. It will then be disabled and deleted. In that + // case, the writer should also report the error via Error(). If a writer + // does not specifically implement one of the methods, it must still + // always return true. - // Called once for initialization of the Writer. Must return false if an - // error occured, in which case the writer will be disabled. The error - // reason should be reported via Error(). + // Called once for initialization of the Writer. virtual bool DoInit(string path, int num_fields, LogField** fields) = 0; - // Called once per entry to record. Must return false if an error - // occured, in which case the writer will be disabled. The error reason - // should be reported via Error(). + // Called once per entry to record. virtual bool DoWrite(int num_fields, LogField** fields, LogVal** vals) = 0; // Called when the buffering status for this writer is changed. If // buffering is disabled, the writer should attempt to write out - // information as quickly as possible even if that may have an - // performance impact. If enabled (which the writer should assume to be - // the default), then it can buffer things up as necessary and write out - // in a way optimized for performance. The current buffering state can - // alse be queried via IsBuf(). + // information as quickly as possible even if doing so may have an + // performance impact. If enabled (which is the default), it can buffer + // things up as necessary and write out in a way optimized for + // performance. The current buffering state can be queried via IsBuf(). // - // A writer may ignore buffering if it doesn't fit with its semantics. - // Still return true in that case. + // A writer may ignore buffering changes if it doesn't fit with its + // semantics. virtual bool DoSetBuf(bool enabled) = 0; // Called when a log output is to be rotated. Most directly, this only @@ -83,7 +83,7 @@ protected: // still call Error(), but return true. // // A writer may ignore rotation requests if it doesn't fit with its - // semantics. In that case, still return true. + // semantics. virtual bool DoRotate(string rotated_path) = 0; // Called once on termination. Not called when any of the other methods @@ -106,13 +106,20 @@ protected: const string Path() const { return path; } private: - // Delete values as passed into Write(). + friend class LogMgr; + + // When an error occurs, we set this flag. The LogMgr will check it an + // remove any disabled writers. + bool Disabled() { return disabled; } + + // Deletes the values passed into Write(). void DeleteVals(LogVal** vals); string path; int num_fields; LogField** fields; bool buffering; + bool disabled; // For Fmt(). char* buf;