Comment # 3
on bug 8917
from Chris Bontje
(In reply to comment #2)
> A few quick questions/comments:
>
> - Using wmem_new instead of wmem_alloc should simplify allocating
> fastser_dataregion structure (removes need for cast and sizeof).
That really is a simple change that also makes the other two existing 'save'
functions cleaner - done.
> - Why do you do use the ret variable:
> ret = dataregion->name;
> return ret;
> instead of just
> return dataregion->name;
> ?
Agreed, done.
> - If you are always going to be looking up dataregions based on
> base_address, you could store them in a wmem_tree instead which will be
> faster and easier than a linked list. (A tree may make sense to replace some
> of your other lists as well, I haven't looked. I only recently added it
> during Sharkfest, so it wasn't available when you originally wrote your
> dissector).
The tree functionality looks cool but from what I've seen in the example in the
DNS dissector, it appears to replace the entire linked list-based conversation
structure - so it would be a pretty big change at this point from the 3 other
portions I have that are LL-based.
> - If you are going to be looking up dataregion structs to get values other
> than the name, it may make sense for the lookup function to just return a
> struct pointer, then you can access the various struct members as
> appropriate.
>
> - You add a bunch of fields to the dataregion struct, but only read two of
> them that I can see. I assume they will be used in a later patch?
The data region will ultimately be likely only used for the name lookup, so
I've removed the other fields for now. If I need them I can always add them
back.
Revised patch attached.
Chris
You are receiving this mail because:
- You are watching all bug changes.
