Evan Huus
changed
bug 8287
| What |
Removed |
Added |
| Attachment #9956 Flags |
review_for_checkin?
|
review_for_checkin-
|
Comment # 19
on bug 8287
from Evan Huus
Comment on attachment 9956 [details]
add links, timespan and number to HTTP dissector
Thanks for the updated patch. Having now had the opportunity to review the
patch in more detail, I also have a few concerns with it.
1. The data structure being used is non-trivial, and there isn't a clear
explanation of its structure anywhere. The way I'm reading it is that it's
basically a pair of dynamically-growing arrays where every element of the first
array is actually two elements (the request fdnum and the response fdnum) with
all of the index offsets for navigating between the two being manually
hard-coded.
With that assumption on the board (please correct me if it's wrong):
2. All the manual index offsets and such are very complicated. They're not
necessarily incorrect, they're just messy. Is there a particular reason you
didn't define something like
struct {
guint32 req_fd, res_fd;
nstime_t ts;
};
and store a single array of those? If I am understanding correctly, it would
greatly simplify much of the code, and make it more readable as well.
3. You loop the entire array several times for each packet, which is
inefficient. All of your various lookup_ functions research the array, when it
should be possible (and much simpler) to find the entry once, return the index,
and just do a straight index the rest of the times.
4. Is an array even the right data structure to use here? Based on the search
patterns I'm seeing in the code, I suspect a hash table or binary tree would be
possible, and algorithmically faster. (Ironically, binary tree's are something
wmem doesn't do yet, so you'd be back to emem if you wanted to make use of
those).
5. I believe the matching will fail if you see packets not in a strict req-res
order, for example REQ1, REQ2, RES1, RES2. This is a perfectly valid order, and
quite common in reality since browsers frequently make several requests
near-simultaneously. Am I missing somewhere where you handle that?
Finally, a few additional trivialities. They don't really affect the
suitability of the patch, but:
- it would be nice if your field-definition entries followed the same
indentation style as the rest of the existing ones in the dissector
- by convention in Wireshark we explicitly specify 'unsigned int' and not just
'unsigned'
- storing the allocator with "wmem_allocator_t *all = wmem_file_scope();" isn't
particulary useful - it adds code, and the compiler will almost certainly
inline the function call anyways since the definition is so trivial.
I'm denying the patch for now based on my concerns above, but I'd be quite
happy to accept it (or an updated one) if you can explain some of your
reasoning.
Thanks,
Evan
You are receiving this mail because:
- You are watching all bug changes.
