Wireshark-dev: Re: [Wireshark-dev] [patch] new dissector for ETHERNET Powerlink
From: David Büchi <[email protected]>
Date: Tue, 22 Aug 2006 10:52:39 +0200
Thanks a lot for your review!
I tried to change the code according to your suggestion, see the attached patch.
Best Regards,
David


--
David Buechi            Zurich University of Applied Sciences Winterthur
Dipl. Ing. FH                         Institute of Embedded Systems InES
Realtime Communication                                Technikumstrasse 9
Tel: +41 52 267 70 60                                        P.O.Box 805
Fax: +41 52 268 70 60                                 CH-8401 Winterthur




ronnie sahlberg schrieb:
decode_epl_adress:
rename it to decode_epl_address
also,  why does this fucntiuon take a string as a parameter and then
just writes a static literal into this string?

this function should be changed to only take adr as a parameter and
return a pointer to a string.  (either ep allocated   or just the
pointer if it is just a static literal (as all strings here actually
are))


same for decode_epl_address_abbrev   it should only take one
parameter: adr and return a pointer to a string


Function names    dont write them as
type function()
split it to two lines like
type
function(...



move proto_register_epl and proto_reg_handoff_epl to the very end of
the file.     these functions are most commonly the very last two
functions in a dissector and it does not hurt to maintain consistency.


all your helper dissectors use hardcoded offsets into the tvb.
can you change this so that the helpers take offset as a parameter
and returns the next offset (isntead of void)   and make the dissector
advance offset accordingly after each field has been dissected.   this
makes the dissector more consistent with how the other dissectors are
written and eases maintenance


why is it a plugin?   nothing wrong with a plugin   but some
developers never compile wireshark with plugins and plugin dissectors
may get less maintenance than normal dissectors.









On 8/19/06, David Büchi <[email protected]> wrote:
Hi,

the attached patch adds support for the ETHERNET Powerlink protocol.
An example capture is attached.

As this is my first contribution to the Wireshark project, I will
appreciate any feedback regarding coding technics/style etc.


Best Regards,
David


--
David Buechi            Zurich University of Applied Sciences Winterthur
Dipl. Ing. FH                         Institute of Embedded Systems InES
Realtime Communication                                Technikumstrasse 9
Tel: +41 52 267 70 60                                        P.O.Box 805
Fax: +41 52 268 70 60                                 CH-8401 Winterthur







Attachment: packet-epl.patch.gz
Description: GNU Zip compressed data