Raúl Pérez Clavero
changed
bug 8275
| What |
Removed |
Added |
| Attachment #10662 is obsolete |
|
1
|
Comment # 17
on bug 8275
from Raúl Pérez Clavero
Created attachment 11093 [details]
New patch file
> - The indentation is still not entirely consistent. Please add modelines to
> the bottom of the file (https://www.wireshark.org/tools/modelines.html).
I've tried to use modelines as requested.
>
> - The preferences implementation is a bit non-standard:
> - You register some obsolete preferences, which is odd since the dissector
> isn't included in any previous versions of Wireshark. Were you distributing
> it as a plugin before? (This is fine, but a comment explaining would be
> helpful).
OK: The obsolete preferences were included by mistake.
> - Usually the proto_reg_handoff_acl function is used as the prefs callback
> (instead of a separate reinit_acl). This is also where dissector handles
> (like acl_handle and xml_handle) should be created or discovered. Take a
> look at the file doc/packet-PROTOABBREV.c for an example of the usual style.
OK: Done!
> - There's no need for the enable_acl_dissection preference. Wireshark
> keeps a master list of enabled/disabled protocols already so that individual
> dissectors don't have to implement that.
OK: Done!
>
> - We already provide an array_length macro defined in packet.h, there's no
> need to have your own acl_array_length.
OK: Done!
>
> - Some of your tables (like acl_performatives and others) can probably be
> made const.
OK: Done!
>
> - Please include in your patch the addition of the dissector to the
> appropriate Makefiles (see section 1.9 of doc/README.developer).
OK: Done
>
> - In some places (such as acl_get_bounds, though there may be others) it
> probably makes sense to use tvb_reported_length() instead of just
> tvb_length(), as this will behave better with captures that were truncated.
OK?: I am not sure I truly understand the difference between the two
functions.
>
> - Finally, it doesn't compile on my Linux system (my compiler is somewhat
> more strict than the default Windows one). Here's a copy of some of the
> errors:
>
OK: I set up a Linux environment and was able to fix all the issues.
Evan, thank you for your comments. I hope this version is closer to what you
are expecting.
Regards.
You are receiving this mail because:
- You are watching all bug changes.