Wireshark · Ethereal-dev: [Ethereal-dev] Re: cimd dissector

[Piros Lucian <lpiros@xxxxxxxxx>]

Hello Ronnie,

Can you please give me some help directions regarding your comment #3?

Thank you

Lucian Piros

On Tue, 2005-08-23 at 07:05 -0400, ronnie sahlberg wrote:
> can you update it to 
> 
> 1, terminate all value_string s properly
> with a {0,NULL} entry.
> 
> 2, You use several char buffers allocated on the stack   such as
> tmpBuffer1[1024]
> Can you change this to #include <epan/emem.h>
> and allocate them with packet temporary scope using ep_alloc()
> instead,   this way   even if there are buffer overflows, the buffer
> overflow will not affect/modify the stack.
> 
> 3, Can you replace the other_decode_bitfield()+proto_tree_add_text()
> with using hf_fields + proto_tree_add_item()+ either a value_string or
> a true_false_string (if it is a boolean).
> This makes the fields fitlerable and makes the dissector much better.
> 
> One such example that would be MUCH nicer if refactored this way is :
> -  switch (oct & 0x03)
> -  {
> -  case 0x00: str = "Class 0"; break;
> -  case 0x01: str = "Class 1 Default meaning: ME-specific"; break;
> -  case 0x02: str = "Class 2 (U)SIM specific message"; break;
> -  case 0x03: str = "Class 3 Default meaning: TE-specific"; break;
> -  }
> -
> -  other_decode_bitfield_value(bigbuf, oct, 0x03, 8);
> -  proto_tree_add_text(subtree, tvb,
> -      startOffset + 1 + CIMD_PC_LENGTH + 1, 1,
> -      "%s :  Message Class: %s%s",
> -      bigbuf,
> -      str,
> -      (oct & 0x10) ?  "" : " (reserved)");
> -  }
> 
> There are many examples of how to do this in other dissectors,  tell
> me if you want me to provide a pointer to a specific instance where
> this is done.
> 
> 
> 4, Can you replace -int hf_index[38];
> with real hf_fields with meaningful names
> This would eliminate the 
> -  static hf_register_info hf[4 + 37] = {
> and the for loop populating it as well.
> 
> 5, Can you do the same with the ett fields and remove that for loop
> populating it and use real ett variables
> with meaningful names instead of
> vals_hdr_PC[i].ett_p
> 
> 
> 6, can you change dissect_cimd() to a new style dissector that returns
> an int ::= number of bytes consumed.
> 
> 
> On 8/23/05, Piros Lucian <lpiros@xxxxxxxxx> wrote:
> > 
> > Hello,
> > 
> > I made the suggested changes. Please find the new diff in attachment.
> > 
> > Regards,
> > Lucian Piros
> > 
> > 
> > On Thu, 2005-08-18 at 09:23 +1000, ronnie sahlberg wrote:
> > > Some initial comments :
> > > 
> > > 1, do not use C++ style comments. not all compilers we use support //
> > comments.
> > > 
> > > 2, do not put value_string definitions in the header file,   put them
> > > in the .c file
> > > 
> > > 3, the cimd_rinfo[]  structure has NULL where there should be a
> > > pointer to the hf_index
> > > declare proper hf_index fields and use them.
> > > 
> > > 4, you have a lot of arrays declared in the ehader file, move all
> > > these to the .c file
> > > 
> > > 5, you use separate arrays for the values and the strings   such as
> > > cimd_vals_PC / cimd_vals_PC_explained.
> > > Rewrite this ald replace all such constructs with a proper
> > > value_string    grep for value_string to see how they are used.
> > > 
> > > 
> > > can you start by doing these changes and resubmit  and i will rereview
> > > the patch.
> > > there are a bunch of other things need fixing too   but start with
> > > these 5 items.
> > > 
> > > 
> > > 
> > > On 8/17/05, Piros Lucian <lpiros@xxxxxxxxx> wrote:
> > > > Hello,
> > > > 
> > > > My name is Lucian Piros and i'm from Romania. I would like to
> > contribute
> > > > to ethereal with a new dissector - cimd dissector. CIMD stands for
> > > > Computer Interface to Message Distribution and it's used to transfer
> > > > short messages between applications and Nokia Short Message Service
> > > > Center.
> > > > Please find the diff output in the attachment. If you find something
> > > > wrong or if you have comments/questions please let me know.
> > > > 
> > > > 
> > > > Thank you and best regards,
> > > > 
> > > > Piros Lucian <lpiros@xxxxxxxxx>
> > > > 
> > > > 
> > > > _______________________________________________
> > > > Ethereal-dev mailing list
> > > > Ethereal-dev@xxxxxxxxxxxx
> > > > http://www.ethereal.com/mailman/listinfo/ethereal-dev
> > > > 
> > > > 
> > > > 
> > > >
> > -- 
> > Piros Lucian <lpiros@xxxxxxxxx>
> > 
> >
-- 
Piros Lucian <lpiros@xxxxxxxxx>