Consistent error handling.

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);

src/LogMgr.cc

@@ -89,6 +89,28 @@ LogMgr::Stream* LogMgr::FindStream(EnumVal* stream_id)
 	return streams[idx];
 	}
 
+void LogMgr::RemoveDisabledWriters(Stream* stream)
+	{
+	for ( list<Filter*>::iterator i = stream->filters.begin(); i != stream->filters.end(); ++i )
+		{
+		Filter* filter = (*i);
+
+		list<string> 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<string>::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));
 	}

src/LogMgr.h

@@ -76,6 +76,7 @@ private:
 	bool TraverseRecord(Filter* filter, RecordType* rt, TableVal* include, TableVal* exclude, string path, list<int> indices);
 	LogVal** RecordToFilterVals(Filter* filter, RecordVal* columns);
 	Stream* FindStream(EnumVal* stream_id);
+	void RemoveDisabledWriters(Stream* stream);
 
 	vector<Stream *> streams; // Indexed by stream enum.
 };

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)

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;