Wireshark · Wireshark-dev: Re: [Wireshark-dev] Source code for ACN (ANSI BSR E1.17) Dissector

["ronnie sahlberg" <ronniesahlberg@xxxxxxxxx>]

Since it wont go into the next release on monday anyway (already branched off)


It produces a lot of compiler warnings when compiling with gcc :


packet-acn.c: In function `dissect_acn_heur':
packet-acn.c:734: warning: implicit declaration of function `memcmp'
packet-acn.c: In function `acn_add_channel_owner_info_block':
packet-acn.c:762: warning: unused parameter `tree'
packet-acn.c: In function `acn_add_channel_member_info_block':
packet-acn.c:797: warning: unused parameter `tree'
packet-acn.c: In function `acn_add_expiry':
packet-acn.c:836: warning: unused parameter `pinfo'
packet-acn.c: In function `acn_add_channel_parameter':
packet-acn.c:851: warning: unused parameter `pinfo'
packet-acn.c: In function `acn_add_cid':
packet-acn.c:874: warning: unused parameter `pinfo'
packet-acn.c: In function `acn_add_address':
packet-acn.c:967: warning: int format, pointer arg (arg 3)
packet-acn.c:967: warning: too many arguments for format
packet-acn.c:910: warning: unused parameter `pinfo'
packet-acn.c: In function `acn_add_dmp_address_type':
packet-acn.c:977: warning: unused parameter `pinfo'
packet-acn.c: In function `acn_add_dmp_address':
packet-acn.c:1018: warning: left-hand operand of comma expression has no effect
packet-acn.c:1018: warning: statement with no effect
packet-acn.c:1023: warning: left-hand operand of comma expression has no effect
packet-acn.c:1023: warning: statement with no effect
packet-acn.c:1000: warning: unused parameter `pinfo'
packet-acn.c: In function `acn_add_dmp_data':
packet-acn.c:1170: warning: unused parameter `pinfo'
packet-acn.c: In function `acn_add_dmp_reason_codes':
packet-acn.c:1427: warning: unused variable `ok_to_process'
packet-acn.c:1417: warning: unused parameter `pinfo'
packet-acn.c: In function `dissect_acn_dmp_pdu':
packet-acn.c:1563: warning: missing braces around initializer
packet-acn.c:1563: warning: (near initialization for `adt.<anonymous>')
packet-acn.c:1564: warning: missing braces around initializer
packet-acn.c:1564: warning: (near initialization for `adt2.<anonymous>')
packet-acn.c:1559: warning: unused variable `param_tree'
packet-acn.c:1554: warning: `address_count' might be used
uninitialized in this function
packet-acn.c: In function `dissect_acn_sdt_wrapped_pdu':
packet-acn.c:1916: warning: unused variable `param_tree'
packet-acn.c: In function `dissect_acn_dmx_data_pdu':
packet-acn.c:2220: warning: missing braces around initializer
packet-acn.c:2220: warning: (near initialization for `adt.<anonymous>')
packet-acn.c:2217: warning: unused variable `addr_tree'
packet-acn.c: At top level:
packet-acn.c:153: warning: `hf_acn_sdt_pdu' defined but not used
packet-acn.c:162: warning: `hf_acn_dmx_dmp_vector' defined but not used

Can you look into eliminating these.


Also, while not a real issue, but in most dissector helpers we use we
return the new value of offset and not the number of bytes consumed.
Could you change your helpers to do this?
so that after calling them it would be
offset=foo(..., offset, ...);

instead of
 offset += foo(...);


This would make it much more inline with other dissectors and reduce
our maintenance workload of this dissector (since it is currently
different to most others).



We also prefer (for consistency) that
the functions :
proto_register_PROTO()
and
proto_reg_handoff_PROTO()
are the last two functions defined in the fiel and not the first two ones
(so that ESC-> will bring you straight there   unless your editor is "broken")



We do suffer in wireshark from port collissions due to the number of
protocols we support.
So a port number is not really enough for us to identify a protocol.
Can you make dissect_acn() do some heuristics and return FALSE if it
didnt really look like ACN in the first place?
This would reduce the probability for false ACN dissection for those
users that have set an ACN port and forgotten about it.
I.e. make dissect_acn() a new style dissector that can refuse the
packet by returning FALSE and return TRUE meaning : yes this was one
of mine and i did dissect it.

The sharper the heuristic filter is the better.


best regards
ronnie s




On 10/25/06, Bill Florac <bill.florac@xxxxxxxxxxxxxx> wrote:
> Ronnie,
>
> Correct files are attached.
>
> I created an ACN wiki page and uploaded a small capture file. (More to
> follow)
>
> Thanks for your time.
>
> Bill
>
>
>
> ________________________________
>
> From: wireshark-dev-bounces@xxxxxxxxxxxxx on behalf of ronnie sahlberg
> Sent: Tue 10/24/2006 5:24 PM
> To: Developer support list for Wireshark
> Subject: Re: [Wireshark-dev] Source code for ACN (ANSI BSR E1.17) Dissector
>
>
> Hi,
>
> Can you try avoiding functions that allocate memory that has to be freed?
> We have ephemeral memory allocators for this purpose.
> I.e.   tvb_get_ephemeral_string instead of tvb_get_string+gfree
>
> There are some // comments in the source. Please replace with C comments
>
>
> Can you also provide a few small example captures to test with and a nice
> page on the WIKI for this protocol?
>
>
> Apart from that it looks good at a first glance.
>
>
>
>
>
>
> On 10/25/06, Bill Florac <bill.florac@xxxxxxxxxxxxxx> wrote:
>
>  Developers,
>
>  Please consider adding the attached source code for dissecting the
> "Architecture for Control Networks", or "ACN" to the Wireshark build. We
> have done a fair amount of internal testing and there a no known bugs. I
> think we complied with all the source code format requirement but if not,
> just let me know and I will gladly fix them. Once posted, I will also submit
> capture files samples.
>
>  It was last tested on build 19109. It was tested as both a built in
> dissector and as a plug-in.  As requested (by Jaap) I am only submitting the
> files for a built-in variant.  There is an existing ACN plug-in that should
> be removed.
>
>  Dissector also includes an option to enable an extension protocol called
> Streaming DMX (ANSI BSR E1.31).  This extension has not been formally
> approved by ANSI yet and is subject to change..
>
>  <<packet-acn.c>> <<packet-acn.h>>
>  Bill Florac
>  Senior Technical Product Specialist
>  bill.florac@xxxxxxxxxxxxxx
>  Electronic Theatre Controls, Inc.
>  3031 Pleasant View Rd .
>  Middleton, WI 53562
>  608-831-4116 (corp. phone)
>
>
>
>  _______________________________________________
>  Wireshark-dev mailing list
>  Wireshark-dev@xxxxxxxxxxxxx
>  http://www.wireshark.org/mailman/listinfo/wireshark-dev