[bugzilla-daemon@xxxxxxxxxxxxx]

Comment # 7 on bug 8673 from Pascal Quantin

(In reply to comment #6)
> Created attachment 10788 [details]
> GVSP dissector patch
> 
> Thanks for the feedback. I've uploaded an updated patch file.

Hi Warren,

I had a closer look at your patch and have a few more comments:

- please use consistent indentation (there is a mix between tabs and spaces)

- the PACKAGE and VERSION defines coming from the plugin version can be removed

- the display_string structure is not used so it can be removed

- formatstring and payloadstring can probably be removed from the
gvsp_packet_info structure as it is used only once

- port 20202 is already registered for IPD Tunneling Port according to IANA
website. Is there an official port number registered? If not, you should add a
preference allowing to modify the port number and have is set to 0 by default.
Alternatively, maybe an heuristic dissector could be used?

- while putting many operations under a (tree != NULL) check can be a good
thing for speed, performing the check fore a single proto_tree_add* call is a
bit overkill in my opinion (as those functions handle being called with a null
tree). Couldn't most of those checks replaced simply by putting the switch on
(info.format) variable in dissect_gvsp() function under a (tree != NULL) check
? It would allow to remove the check in all sub functions.

- here are the errors found by tools/checkfiltername.pl script:
gvcpzonedirection doesn't match PROTOABBREV of gvsp
gvcp.zone0direction doesn't match PROTOABBREV of gvsp
gvcp.zone1direction doesn't match PROTOABBREV of gvsp
gvcp.zone2direction doesn't match PROTOABBREV of gvsp
gvcp.zone3direction doesn't match PROTOABBREV of gvsp
gvcp.zone4direction doesn't match PROTOABBREV of gvsp
gvcp.zone5direction doesn't match PROTOABBREV of gvsp
gvcp.zone6direction doesn't match PROTOABBREV of gvsp
gvcp.zone7direction doesn't match PROTOABBREV of gvsp
gvcp.zone8direction doesn't match PROTOABBREV of gvsp
gvcp.zone9direction doesn't match PROTOABBREV of gvsp
gvcp.zone10direction doesn't match PROTOABBREV of gvsp
gvcp.zone1direction doesn't match PROTOABBREV of gvsp
gvcp.zone12direction doesn't match PROTOABBREV of gvsp
gvcp.zone13direction doesn't match PROTOABBREV of gvsp
gvcp.zone14direction doesn't match PROTOABBREV of gvsp
gvcp.zone15direction doesn't match PROTOABBREV of gvsp
gvcp.zone16direction doesn't match PROTOABBREV of gvsp
gvcp.zone17direction doesn't match PROTOABBREV of gvsp
gvcp.zone18direction doesn't match PROTOABBREV of gvsp
gvcp.zone19direction doesn't match PROTOABBREV of gvsp
gvcp.zone20direction doesn't match PROTOABBREV of gvsp
gvcp.zone21direction doesn't match PROTOABBREV of gvsp
gvcp.zone22direction doesn't match PROTOABBREV of gvsp
gvcp.zone23direction doesn't match PROTOABBREV of gvsp
gvcp.zone24direction doesn't match PROTOABBREV of gvsp
gvcp.zone25direction doesn't match PROTOABBREV of gvsp
gvcp.zone26direction doesn't match PROTOABBREV of gvsp
gvcp.zone27direction doesn't match PROTOABBREV of gvsp
gvcp.zone28direction doesn't match PROTOABBREV of gvsp
gvcp.zone29direction doesn't match PROTOABBREV of gvsp
gvcp.zone30direction doesn't match PROTOABBREV of gvsp
gvcp.zone31direction doesn't match PROTOABBREV of gvsp

- tools/checkhf.pl script identified that hf_gvsp_chunklayoutid is used but not
defined. Please define the corresponding array entry.

- tools/fix-encoding-args.pl identified the following errors:
FT_BYTES:         proto_tree_add_item(gvsp_tree, hf_gvsp_payloaddata, tvb,
offset, -1, [[ENC_BIG_ENDIAN]-->[ENC_NA]]);
FT_BYTES:         proto_tree_add_item(gvsp_tree, hf_gvsp_payloaddata, tvb,
offset + 8, -1, [[ENC_BIG_ENDIAN]-->[ENC_NA]]);
FT_BYTES:         proto_tree_add_item(gvsp_tree, hf_gvsp_payloaddata, tvb,
offset + 8, -1, [[ENC_BIG_ENDIAN]-->[ENC_NA]]);
FT_STRINGZ:       proto_tree_add_item(gvsp_tree, hf_gvsp_filename, tvb, offset
+ 20, -1, [[ENC_BIG_ENDIAN]-->[ENC_ASCII|ENC_NA]]);
REG_PROTO:        proto_tree_add_item(tree, proto_gvsp, tvb, offset, -1,
[[ENC_BIG_ENDIAN]-->[ENC_NA]]);

Regards,
Pascal.

---

      You are receiving this mail because:

• You are watching all bug changes.