[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=6373

--- Comment #9 from Yaniv Kaul <mykaul@xxxxxxxxx> 2011-09-28 00:06:51 PDT ---
(In reply to comment #8)
> Additional notes:  :)

Thank you for the review!

> 
> 1. There are a number of unreferenced value_string arrays:
>    main_cap, playback_fmt_vals, warn_codes, info_codes, ...

Fixed.

> 
> 2. The preferred source file format: proto_register...() followed by
> proto_reg_handoff...() at the end of the file.

Fixed.

> 
> 3. TCP ports 5911 & 5912 do not appear to be assigned to SPICE in the IANA port
> list but appear to be assigned for something else. Are there IANA assigned
> ports for SPICE ?
> 
>  See:
> http://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xml
> 
> If there are no assigned ports for SPICE, then just use the heuristic and
> possibly the "alternate port" preference.

Fixed - but see the issue I have below with VNC.

> 
> 4. The "alternate port" preference is commented out but the related code and
> variables are not.  Do you want the alternate port preference capability ?
> 
> If yes, please see the packet-bvlc.c dissector for an example of how to do this
> (which is slightly different from that in packet-spice.c).

Fixed - deleted.

> 
> 5. spice.magic is shown as FT_STRING so ENC_NA is the correct encoding in the
>    proto_tree_add_item() call.

Fixed.

> 
> 6. Given that the magic field is a string, please use tvb_memeql() when testing
> the field against "REDQ".

No need - this is the way it's done in the Spice code as well. It is a UINT32.

> 
> 7. It appears that all the usage of val_to_str() has a constant string as the
> 3rd arg.  Using val_to_str_const() instead in these cases is an optimization
> which will save some memory allocation behind the scenes.

Fixed.

> 
> 8. proto_reg_handoff(): The 'find_dissector("image-jfif")' should be under the
>    'if(!initialized)' since it needs to be called only once.

Fixed.

> 
> 9. $Id$ is missing from the file.
> 
>    (See doc/README.developer); (Search for $Id$).

Fixed.

> 
> 10. 'proto_tree_add_item(tree, proto_spice, ...)' should use ENC_NA in all
> cases.

Fixed.

> 
> 11. There are several uses of tvb_length...() which I think should be
> tvb_reported_length...().  Did you use tvb_length...() in these specific cases
> for a reason ?

Fixed.

> 
> 12. Almost all the ints displayed using proto_tree_add_item() are
> ENC_LITTLE_ENDIAN.
> 
> A few are ENC_BIG_ENDIAN. Is this correct ?
> 
> Specifically: hf_session_id, hf_LZ_major_version, hf_LZ_minor_version

Regretfully, this is correct.

> 
> 13. #include <stdio.h> & #include <string.h> aren't needed.
>     #include <.../packet-tcp.h> isn't needed ?

Fixed.

> 
> 14. checkhf.pl gives
> 
>      perl ...\checkhf.pl packet-spice.c
>      unused entry: packet-spice.c, hf_cursor_data_size
>      unused entry: packet-spice.c, hf_Clip_data

Fixed.

> 
> 15. The attached capture file, when opened (on Windows) in Wireshark (with or
> without the SPICE dissector) shows many frames which show as VNC but which are
> not dissected further (even for VNC).
> 
> Is there something special which must be done to enable SPICE dissection ?

Two options:
1. Delete the association between port 5900 and VNC - as VNC has a heuristic
dissection (I've added it in the past)
2. 'Decode As' and select Spice (which is what I've done). However, 'Decode As'
won't work if the dissector is only heuristic (which sounds like a bug to me) -
I had to do:
    dissector_add_uint("tcp.port", 0, spice_handle);
to have it on the list...
3. Fix VNC dissector, that even if it's registered on that port, it won't grab
it if it can't find VNC traffic?! 'Un-register' it in that case? not sure how
to do it exactly.

> 
> 16. Whitespace:
>     - Some lines have trailing whitespace; it would be appreciated is this is
>        removed. 
>     - What is the intended indentation width: 4 or 8 ? 
>       If 4, then please use spaces iso "4 space tabs".
> 

Fixed.

> 17. Has the dissector been fuzz-tested ?

No, is there some automated manner of doing it? Wasn't sure how to get
fuzz-test.sh work against it:
[ykaul@ykaul wireshark]$ tools/fuzz-test.sh -p 10 ~/spice-example.pcap.gz 
Error: No valid capture files found.

-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.