Wireshark-bugs: [Wireshark-bugs] [Bug 8431] New Dissector Patch - USB PTP / MTP
Date Index · Thread Index · Other Months · All Mailing Lists
Date Prev · Date Next · Thread Prev · Thread Next
From: bugzilla-daemon@xxxxxxxxxxxxx
Date: Fri, 08 Mar 2013 12:54:48 +0000

Comment # 13 on bug 8431 from 
I took a quick look. Here are some comments:

packet-usb-ptp.h:
- I wasn't expecting to see array declarations in a .h file
- hf_ prefixes are usually reserved for field handles -
  it's confusing to use them for string arrays
- "libghoto2" should be "lipgphoto2"?

packet-usb-ptp.c:
- Functions except the last 2 should all be 'static'
- usb_ptp_flavor(): consider implementing as switch
- dissect_usb_ptp_params():
    'while (remaining && remaining >= 4)': can remove 'remaining &&'
- dissect_usb_ptp():
    * remove _U_ notation from ptp_length
    * Difficult to read offset increment statements on same line as other code
    * Consider expanding "col_class" to make the Info column display a little
      more intuitive for novice users, i.e. "CMD/RSP/DAT/EVT" instead of 
      "C/R/D/E"
- proto_register_usb_ptp()
    * hf[]: Use VALS() for the 'strings' field

In general I think more whitespace would make the code easier to review, i.e.:
offset=usb_ptp_add_array_is(pinfo,tree,hf_devinfo_operationsupported ...
-->
offset = usb_ptp_add_array_is(pinfo, tree, hf_devinfo_operationsupported ...

You are receiving this mail because:
  • You are watching all bug changes.