Comment # 23
on bug 8287
from Evan Huus
(In reply to comment #21)
> your comments were helpful in rethinking the current state. My previous code
> has evolved over time as more "lookup" stuff was added. I agree that
> everything should be combined into a simple struct object describing the
> state of a request/response. I've rewritten that part and the code looks
> much better. It now only has a single lookup function which can return
> pointers to the previous, current and next struct with one iteration. I've
> also used a single linked list, this has ironically removed the need for
> wmem_realloc() in this code.
Yes, this is much better. Having seen this and reread my previous comments, I
think we can do even better by using p_add_proto_data and p_get_proto_data to
store the req/res structure directly with the frame metadata. This should
eliminate the need to scan the list entirely. You will have to make the list
doubly-linked in order to be able to find the previous req/res combo in this
case, but I don't think that's a huge problem. Thoughts?
> If the capture file has too much packet loss and retransmission and the TCP
> stream reassembly is not clean, my request/response tracking can get
> confused. I think however that our users are aware that with problematic
> capture files Wireshark's analysis may be flawed and there's also nothing I
> can do about it.
In general I agree. It might be worth adding a check in push_res that we're
actually expecting a response. Right now, if we get REQ, RES, RES then the
second RES will overwrite the first which isn't necessarilly right (presumably
the first should stay and the second should be marked as an error or
something).
> I've included your suggestions, except for reformatting the definitions of
> the new hf_* variables. I personally don't like the arbitrary line breaks
> used in a lot of existing dissectors and prefer to have one hf_ variable
> defined on a single line. (If whoever likes to commit these changes
> eventually has another strong opinion I'm fine if the comitter reformats
> those)
I don't have a particularly strong opinion about which way is best, but I do
like consistency and don't feel like reformatting all the other entries to
match. And keeping things within 80 columns still feels like a good thing even
though it's not really necessary these days.
You are receiving this mail because:
- You are watching all bug changes.
