Comment # 8
on bug 8275
from Evan Huus
Hi Raúl, I haven't had time to do a full review, but I have a few quick
comments and questions after skimming the patch:
- Why do you put acl-parser.c in its own file? Are you planning on using it in
dissectors other than FIPA/ACL? If you're only planning on using it in the one
place then you might as well merge them all together and save yourself some
extra files.
- I don't think you need packet-acl.h. You're not exposing any functions to
other dissectors, so there's no need for it.
- You make a lot of use of manually-managed memory, via manual allocation calls
(g_new), data structures (GSList), and wireshark API calls (tvb_get_string).
That's usually discouraged, as it is much easier to avoid leaks when using
Wireshark's memory manager (see doc/README.malloc in 1.8, or doc/README.wmem in
trunk).
- There appear to be a few places of inconsistent indentation. Adding modelines
from https://www.wireshark.org/tools/modelines.html might help.
- By convention the proto_register and proto_reg_handoff functions are the last
two functions in the file, as we have some build scripts that may rely on it.
- I'm not sure what you're doing with the port registrations and subdissector
tables etc (again, partly because there are no other protocols that use them at
the moment). What were you trying to accomplish?
Cheers,
Evan
You are receiving this mail because:
- You are watching all bug changes.