New dissector: performance co-pilot protocol
Created attachment 7902 [details] Patch implementing PCP protocol dissection Build Information: TShark 1.7.1 (SVNRev 41198 from /trunk) Copyright 1998-2012 Gerald Combs <gerald@wireshark.org> and contributors. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. Compiled (64-bit) with GLib 2.24.1, with libpcap, with libz 1.2.3.3, without POSIX capabilities, without SMI, without c-ares, without ADNS, without Lua, without Python, without GnuTLS, without Gcrypt, with MIT Kerberos, without GeoIP. Running on Linux 2.6.32-33-generic, with locale en_AU.UTF-8, with libpcap version 1.0.0, with libz 1.2.3.3. Built using gcc 4.4.3. -- Attached patch implements dissection of the Performance Co-Pilot (PCP) protocol. Further details and sample capture file: http://wiki.wireshark.org/PCP This sample capture contains all PDUs, and fuzz-test.sh has been run successfully for over 20000 iterations.
Created attachment 7904 [details] Additional patch to properly decode the results Additional patch. Fixes the results to be displayed properly for each type of instance(int/uint/init64/uint64/etc...)
Hi, Quick Review : Missing modification for Cmake (epan/CmakeFiles.txt) Use FALSE / TRUE for encoding attribut of proto_tree_add_item is obsolete, you need to use ENC_BIG_ENDIAN/ENC_LITTLE_ENDIAN (there is the fix-encoding-args tools for help you) Add also modeline information And for finish, there is some Clang Warning : packet-pcp.c:526:2: warning: Value stored to 'bits_offset' is never read packet-pcp.c:1061:2: warning: Value stored to 'bits_offset' is never read packet-pcp.c:1089:10: warning: Value stored to 'bits_offset' during its initialization is never read packet-pcp.c:1140:2: warning: Value stored to 'bits_offset' is never read packet-pcp.c:1475:5: warning: Value stored to 'offset' is never read packet-pcp.c:1438:6: warning: Value stored to 'offset' is never read packet-pcp.c:1483:5: warning: Value stored to 'offset' is never read packet-pcp.c:1451:5: warning: Value stored to 'offset' is never read packet-pcp.c:1463:5: warning: Value stored to 'offset' is never read packet-pcp.c:1447:5: warning: Value stored to 'offset' is never read packet-pcp.c:1491:5: warning: Value stored to 'offset' is never read packet-pcp.c:1471:5: warning: Value stored to 'offset' is never read packet-pcp.c:1459:5: warning: Value stored to 'offset' is never read packet-pcp.c:1467:5: warning: Value stored to 'offset' is never read packet-pcp.c:1487:5: warning: Value stored to 'offset' is never read packet-pcp.c:1455:5: warning: Value stored to 'offset' is never read packet-pcp.c:1443:5: warning: Value stored to 'offset' is never read packet-pcp.c:1436:6: warning: Value stored to 'offset' is never read packet-pcp.c:1428:5: warning: Value stored to 'offset' is never read packet-pcp.c:1479:5: warning: Value stored to 'offset' is never read packet-pcp.c:1508:2: warning: Value stored to 'offset' is never read
Comment on attachment 7904 [details] Additional patch to properly decode the results And about this patch Why use proto_tree_add_int/float... and not directly proto_tree_add_item?
Created attachment 7944 [details] Updated patch for inclusion Thanks for the review so far I've uploaded a new patch for review which obsoletes the existing two (is this the preferred way or would you prefer patch sets?). I've changed all instances proto_tree_add_item to use the ENC_* constants. Regarding the Clang warnings, I've changed most of the decoding routines to return void as the returned offset is not used again. I've still elected to increment the offset after it has passed through the dissecting routines purely for consistency sake. I did not have any warnings compiling this using GCC 4.6.2. Regarding the use of proto_tree_add_int/float it seems like I needed to do this to dissect non-contiguous bytes in the packet but keep the values stored under the same tree (see comment at line 928 of packet-pcp.c). Depending on the "valfmt type", the 4 bytes stored after the instance (line 912 of packet-pcp.c) is either the value _or_ a pointer to further in the packet of another data structure which contains the value.
Hi, (In reply to comment #4) > Created attachment 7944 [details] > Updated patch for inclusion > > Thanks for the review so far > > I've uploaded a new patch for review which obsoletes the existing two (is this > the preferred way or would you prefer patch sets?). Prefer new patch ! > > I've changed all instances proto_tree_add_item to use the ENC_* constants. > Regarding the Clang warnings, I've changed most of the decoding routines to > return void as the returned offset is not used again. I've still elected to > increment the offset after it has passed through the dissecting routines purely > for consistency sake. I did not have any warnings compiling this using GCC > 4.6.2. I will check ! > > Regarding the use of proto_tree_add_int/float it seems like I needed to do this > to dissect non-contiguous bytes in the packet but keep the values stored under > the same tree (see comment at line 928 of packet-pcp.c). > > Depending on the "valfmt type", the 4 bytes stored after the instance (line 912 > of packet-pcp.c) is either the value _or_ a pointer to further in the packet of > another data structure which contains the value. There is no problem ! proto_tree_add_item can be used
Created attachment 7953 [details] PCP Protocol dissector - remove use of proto_tree_add_(init|uint...) Thanks for the info, I've updated the patch to use proto_tree_add_item instead of proto_tree_add_(int|uint|float...) and have tested it is working as before. Please let me know if there is anything else required to get this merged. Kind regards, Ryan
Thanks ! Committed inRevision 41353 with small change (See revision log)
Coverity reports a missing break between the PM_TYPE_AGGREGATE and PM_TYPE_AGGREGATE_STATIC cases of the switch statement in dissect_pcp_message_result() around line 986. It looks like the code is identical for both cases. So, should the proto_tree_add_item() call be deleted from the PM_TYPE_AGGREGATE case and allow execution to fall-through to PM_TYPE_AGGREGATE_STATIC, or should there be some difference in the code, in which case a break; statement should be added and the the code fixed?
Created attachment 8099 [details] PM_TYPE case fallthough Attached is the patch to allow PM_TYPE_AGGREGATE to fall through to PM_TYPE_AGGREGATE_STATIC. Cheers
Comment on attachment 8099 [details] PM_TYPE case fallthough Fix committed inRevision 41812
