Wireshark-bugs: [Wireshark-bugs] [Bug 8282] DICOM dissector: Extended Negotiation support missin
Comment # 8
on bug 8282
from Stefan Allers
Hi Evan,
thanks for the review! Please find my remarks below.
In the meantime there were some code changes (with conflicts) checked-in by
someone else. So my new patch is based on the latest changes.
(In reply to comment #7)
> 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.
Done
> - 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?
Sorry, that was accidental and I also oversaw this in the diff
> - 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.
OK - changed to proto_tree_add_item and ENC_BIG_ENDIAN for 16 bit 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.
Done. Thanks for the hint.
> 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.
