Evan Huus
changed
bug 8282
| What |
Removed |
Added |
| Attachment #9927 Flags |
review_for_checkin?
|
review_for_checkin-
|
Comment # 7
on bug 8282
from Evan Huus
Comment on attachment 9927 [details]
Patch to support Ext. Neg. for Query/Retrieve SOP Classes (dicom dissector) - 2
Hi Stefan,
Thanks for the patch and the capture file, and sorry it's taken me so long to
get round to reviewing them. I just have a few minor comments:
- Please use consistent indentation. Part of the patch uses spaces, and part
uses tabs, which (at least on my display) makes them not line up at all. The
rest of the file uses 4-spaces indentation so you should probably stick with
that. Adding modelines (https://www.wireshark.org/tools/modelines.html) might
help.
- You rename an argument in the prototype of the dcm_set_syntax function but
don't rename the argument in the function definition. I assume that this was
accidental?
- For item_type, item_len and sop_class_uid_len you don't need to use
proto_tree_add_uint. The proto_tree_add_item function will work just as well
(and is simpler/faster), since the values you're adding are not generated but
are available in the packet. The proto_tree_add_uint function (and similar ones
for other types) are typically only needed for generated values.
- The 'cnt' variable can underflow if sop_class_uid_len is larger than
item_len. I suggest making cnt a gint32 and replacing all the ==0 tests with
<=0.
Other than that it looks pretty good. I'm denying this particular version of
the patch because of the underflow, but I'll be happy to accept a fixed
version.
Cheers,
Evan
You are receiving this mail because:
- You are watching all bug changes.