[Bro-Commits] [git/bro] topic/robin/reporting: Overhauling the internal reporting of messages to the user. (93894ee)

How are errors in the script layer handled? If there's an error in a script you can't really call an event handler for to handle that error, since it might well be that the error is either in the event handler or that the error will prevent the event handler from working properly.

cu
gregor

It's not really *handled*, it just raises a new log_error event that
reports the error with a message. That event is then just queued
normally, which shouldn't cause any further trouble.

(Well, I guess the handler for log_error better doesn't cause any new
errors; but even if, it will just flood the log file, Bro will keep
processing.)

Robin

I really like the idea of having event handlers to handle error messages! A couple more comments:

Can the log_* handlers also write to stderr instead of stdout? I think that would be better.

It appears that there are several places, esp. in Expr.cc and Stmt.cc that still use the old Error(), Internal(), etc. functions (including the SetError). Is this on purpose?

How are errors in the script layer handled? If there's an error in a
script you can't really call an event handler for to handle that
error,

It's not really *handled*, it just raises a new log_error event that
reports the error with a message. That event is then just queued
normally, which shouldn't cause any further trouble.

I was more wondering what would happen if due to an error the handler for log_error cannot run properly. I.e., whether there can be a case were we don't see any error messages.

(Well, I guess the handler for log_error better doesn't cause any new
errors; but even if, it will just flood the log file, Bro will keep
processing.)

Well, log_error will go in an endless loop and continue to queue events. So bro won't process any more packets because the event queue never gets empty, right?

cu
Gregor

Can the log_* handlers also write to stderr instead of stdout?

The current implementation is just a placeholder. The output should
probably go into its own log file but I let Seth figure that out. :slight_smile:

It appears that there are several places, esp. in Expr.cc and Stmt.cc
that still use the old Error(), Internal(), etc. functions (including
the SetError). Is this on purpose?

Yes, these are BroObj's Error() etc. methods; I just removed the
methods in the util.h. Generally, I left object-specific error methods
in because (1) they sometimes do additional things, and (2) it would
have required touching lots of places without making things much
better.

But: these methods used to call error() for doing their output and
*that* has been replaced with Logger::Error().

Let me know if you still find a method logging errors directly to
stderr/out (except for debugging stuff, I didn't touch that, like in
Rule*.cc).

I was more wondering what would happen if due to an error the handler
for log_error cannot run properly. I.e., whether there can be a case
were we don't see any error messages.

Can't think of anything.

Well, log_error will go in an endless loop and continue to queue events.

Hmm, that's right. I forgot that we aren't limiting how many events we
process each time we drain the manager (we do limit timers per
packet). Seems that should be caught and errors reported in a
log_error hanlder be written out to stderr.

Another question: what do you guys think about merging Logger and
LogMgr now? I.e., let LogMgr do both error reporting and normal log
output.

Robin

Another question: what do you guys think about merging Logger and
LogMgr now? I.e., let LogMgr do both error reporting and normal log
output.

Are you talking about just merging the Logger's currently implemented interface as-is under the LogMgr class, or was there some additional integration of the two you were thinking about? If the former, it seems like maybe just finding a better name for the Logger class would do (MsgReporter, maybe?).

- Jon

Are you talking about just merging the Logger's currently implemented
interface as-is under the LogMgr class,

Just as-is. I'm kind of torn whether the two belong together or not.

(MsgReporter, maybe?).

Yeah, I can see that.

Robin