Comment # 10
on bug 8275
from Raúl Pérez Clavero
Hi,
Thank you for your comments Evan.
(In reply to comment #8)
> - 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.
While developing the dissector, it was easier for me to keep the parsing logic
from the dissection logic decoupled. I used acl-parser's main function to test
the acl parser.
>
> - 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 are right. I can be removed or left in case any function is exposed.
> - 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).
It shouldn't be too hard to use emem. I'll look into it.
>
> - There appear to be a few places of inconsistent indentation. Adding
> modelines from https://www.wireshark.org/tools/modelines.html might help.
>
I'll have a look
> - 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.
No problem.
> - 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?
>
The ACL does not have a specific port in which it operates. I have used ranges
to allow the user scan for ACL messages in any port.
I should have removed the sub dissector table registration. Sorry for the
confusion.
You are receiving this mail because:
- You are watching all bug changes.
