Michael Mann
changed
bug 8564
| What |
Removed |
Added |
| Attachment #10551 Flags |
review_for_checkin?
|
review_for_checkin-
|
Comment # 3
on bug 8564
from Michael Mann
Comment on attachment 10551 [details]
The patch file
DeviceNet dissector review
"Coding standards"
1a. Whitespace formatting is inconsistent, please correct and add editor
modelines (http://www.wireshark.org/tools/modelines.html)
1b. Trailing whitespace found
2. Run checkhf.pl
3. Run checkAPIs.pl. It looks like a bunch of the blurbs should be NULL
because they don't provide a different description than the name itself.
4. Run checkfiltername.pl. The "cip" filters are probably acceptable, but the
"msg" filter names should have the "devicenet" prefix
5. Most of the proto_tree_add_* calls can be a single line of code and would
add readability.
Dissector functionality
1. packet-devicenet.h is not necessary. None of the functions are called
outside of the DeviceNet dissector (in fact dissect_devicenet is defined as
static in packet-devicenet.c). The CIP service codes can be found in
packet-cip.h
2. There are a bunch of value_strings and arrays that can (should) be shared
with the CIP dissector (by possibly including them in packet-cip.h as externs).
The ones I found at quick glance are:
a) Vendor ID list. I didn't think the DeviceNet vendor ID list was independent
of the CIP vendor ID list. I thought they were shared (or could at least be
merged).
b) devicenet_class_names_vals should just use cip_class_names_vals
C) You should be able to use the GENERIC_SC_LIST #define in packet-cip.h to
minimized "shared" services in the value_strings
d) cip_attribute_vals
3. The class/instance/attribute values aren't added to the dissection tree or
made filterable.
4. There should be a "default" for a node's bodytype (8/8?), and a UAT array
created for the few nodes that a user may be interested in. 64 enumerated
preferences is a bit of overkill. What would you have done if CAN supported
128 nodes?
5. A lot of the dissection is just putting bytes into a string with no fields.
That's not terribly helpful as the byte output can already be seen in the byte
pane.
6. A few of the proto_tree_add_text can be converted to proto_tree_add_items
(specifically the Vendor IDs)
It's a good start, but cleaning up the whitespace and CIP duplication will make
future reviews easier.
You are receiving this mail because:
- You are watching all bug changes.
