On 5/29/07, jmzhou.ml@gmail.com <jmzhou.ml@gmail.com> wrote:
Some comments about the patch set

Patch 4: I can confirm that this is a bug. I had the same problem and
same fix at my side.

Patch 5: I think the new call (DoGenParseCode) in pac_type.cc should be
replaced by GenParseCode3. Imagine that a record field (identifier f1)
is empty type and there are other fields (identifier f2, f3, ...) after
field f1. If you call DoGenParseCode, the identifier f1 will not be
evaluated like in GenParseCode3 (pac_type.cc:754-755). This will result
in re-entrance of RecordDataField::GenParseCode (pac_record.cc:420) because
when f2->prev()-GenParseCode (pac_record.cc:428) is called, env->Evaluated(
id()) will return false. This is an infinite loop - till at some point of
binpac there will be an assertion failure.

I think I can see your point and also that it might generally be more safe to call GenParseCode3 instead (I attached the updated patch file).
 
Patch 6: I think there is another issue here: if some buffering request
has been sent to the flow buffer, and the flow buffer is not ready with
partial data, we will end up with entering the loop - a waste of CPU
cycles.

Maybe you are right, but what would you suggest as change? Do you want to check whether the buffer is ready before entering the loop? But then it has to be ensured that the buffer is properly initialized in any case.  At the moment I cannot see all the consequences of such a change. And do you think that the impact on performance is really relevant?

Thanks for the comments,
Tobias 

_______________________________________________
Bro mailing list
bro@bro-ids.org
http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/bro