Evan Huus
changed
bug 8414
| What |
Removed |
Added |
| See Also |
|
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7933
|
Comment # 12
on bug 8414
from Evan Huus
Hi Steve, thanks for the patch. I'll do a more thorough review once bug #8339
has been merged, but I have a couple of very minor notes off the top of my
head:
- since you have a link to the spec, please put it directly in a comment in the
code somewhere
- regarding "Forked from packet-usb-masstorage.c" you probably also want to
reference Ronnie Sahlberg (2006) as the original author of that code
- I'm not a huge fan of the static arrays that just contain consecutive
addresses for other arrays, but I think this is more appropriate to discuss in
bug #8339?
- You may want to replace your commented-out boost assertions with
DISSECTOR_ASSERT calls?
- It might be good to put a final 'else' in after "else if (subtype ==
VC_ENCODING_UNIT)" in order to catch cases of an unexpected subtype. An expert
info warning would be the usual choice.
- You've got an if (0) by the comment: @todo Display "N/A" if Camera Terminal
does not support scanning mode control. Should that whole thing be commented
out?
- Not entirely sure what the intended usage of dissect_usb_vid_descriptor is.
Would a uint dissector table make more sense than a manual switch statement?
Also, the use of a do {} while(0) with break statements is clever but
confusing. Simplifying it would be appreciated.
- When setting columns, you can use col_append_fstr instead of an
ep_strdup_printf and a col_append_str.
- Patch includes an extra blank line in Makefile.common?
In general, there's a lot of *very* good work here. It's a big, complicated
dissector, but it's well structured and easy to follow.
Cheers,
Evan
You are receiving this mail because:
- You are watching all bug changes.
