Wireshark · Ethereal-dev: [Ethereal-dev] FW: DIS dissector

["Ouellette, Jeremy J (N-Scientific Research Corp)" <jeremy.j.ouellette@xxxxxxxx>]

Resending... it appears that my original .zip attachment was blocked by
the outgoing mail server; apologies if it's a duplicate.

-----Original Message-----
From: Ouellette, Jeremy J (N-Scientific Research Corp) 
Sent: Monday, April 18, 2005 1:31 PM
To: Ethereal development
Subject: RE: DIS dissector


Ronnie-

I've addressed all of the coding style changes you suggested (moved
pointer asterisks next to variable names, single line function
declarations) and fixed the offsets to pass by value instead of by
pointer.  I see your point about the use of proto_tree_add_text and hope
to have additional opportunity to work on enhancing this dissector to
address these concerns once it's checked into the baseline.
I'm attaching the SVN diff against the latest revision (14128).  Please
let me know if you have any more questions or concerns.

Thanks,
Jeremy Ouellette
Scientific Research Corporation

-----Original Message-----
From: ronnie sahlberg [mailto:ronniesahlberg@xxxxxxxxx] 
Sent: Friday, April 08, 2005 7:03 AM
To: Ouellette, Jeremy J (N-Scientific Research Corp)
Cc: Ethereal development
Subject: Re: DIS dissector


On Thu, 07 Apr 2005 10:29:41 -0400, "Ouellette, Jeremy J (N-Scientific
Research Corp)" <jeremy.j.ouellette@xxxxxxxx> wrote:
> Ronnie-
> 
> Thanks for the feedback.  Obviously some of the suggestions will be
> easier to implement than others -- I've already modified it so that
> offsets are passed by value, not address, and the style-related
changes
> are simple as well.

nice.

> Are these all must-haves for this protocol to be considered for
> inclusion?  

There are no musts as far as i know when it comes to ethereal.
When time permits  some of the ethereal guys can give quick reviews
and comment on committed code.
There are no discussions among ethereal developer before checking
anything in 

If i request some changes before checking it in it only means i as an
individual requested them.   if someone else thinks they are fine as
is and checks it in as is, so be it.


I am aware my suggestions are quite work intensive for your patch  
but it would reduce future maintenance work on the dissector to reduce
the amount of special structures and code used.

If you really cant do those changes,   do the changes that you can
(the passing offset by pointer is disgusting and must be changed)  and
resubmit what you have.


Worst thing that can happen is that your dissector is different from
the others and thus suffer greater risk of bit-rot creeping in than
the normal ones.


Also   change the function headers to stay on  one line.
This 
function(
    type   variable,
...
)

Thing reminds me too much of the bad old days with kr or kr
compatibility mode in compilers.  It is also inconsistent with teh way
function headers are written in the rest of ethereal.

<crazy old mans rant>


Personally i hate proto_tree_add_text() and have often though about
rewriting all 700+ or so calls of this function  so i can delete it so
that it will not be used any more.
Using proto_tree_Add_text() amkes the dissector sub-standard since
filtering is no longer possible.   Removing this function completely
removes this shortcut.



maybe we need a coding style doco for ethereal?
</crazy old mans rant>


Please do the changes you can (offset change is mandatory as far as i
can say   but i am just one individual == I will not check it in
without the offset change, others might) and resubmit.



> In particular, I realize that the overuse of
> proto_tree_add_text() isn't ideal and had noted it as such in my
> comments in packet-dis.c.  However, I believe that in its current
state,
> the dissector is already very useful to the simulation community, and
> adding it to the source tree would provide a mechanism to begin making
> your suggested changes as well as extending the dissector to cover
> additional PDU types as needed by other users.
> 
> Thanks,
> Jeremy Ouellette
> Scientific Research Corporation
>

Attachment: packet-dis.patch.gz
Description: packet-dis.patch.gz