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

["Anders Broman" <a.broman@xxxxxxxxx>]

Hi,
You could do something like in packet-gsm_a.c

/*
 * [3] 10.5.2.21aa MultiRate configuration
 */
/*	Multirate speech version Octet 3 Bits 8 7 6 */
static const value_string multirate_speech_ver_vals[] = {
	{ 1,		"Adaptive Multirate speech version 1"},
	{ 2,		"Adaptive Multirate speech version 2"},
	{ 0,	NULL }
};
/* Bit	5 	NSCB: Noise Suppression Control Bit */
static const value_string NSCB_vals[] = {
	{ 0,		"Noise Suppression can be used (default)"},
	{ 1,		"Noise Suppression shall be turned off"},
	{ 0,	NULL }
};
/* Bit	4	ICMI: Initial Codec Mode Indicator */
static const value_string ICMI_vals[] = {
	{ 0,		"The initial codec mode is defined by the implicit
rule provided in 3GPP TS 05.09"},
	{ 1,		"The initial codec mode is defined by the Start Mode
field"},
	{ 0,	NULL }
};
/*
Table 10.5.2.21aa.2: Set of adaptive multirate codec modes field (octet 4)
for the Multirate speech version 1
*/
static const true_false_string gsm_a_rr_set_of_amr_codec_modes  = {
  "is part of the subset",
  "is not part of the subset"
};
:
:
	proto_tree_add_item(tree, hf_gsm_a_rr_multirate_speech_ver, tvb,
curr_offset, 1, FALSE);
	proto_tree_add_item(tree, hf_gsm_a_rr_NCSB, tvb, curr_offset, 1,
FALSE);
	proto_tree_add_item(tree, hf_gsm_a_rr_ICMI, tvb, curr_offset, 1,
FALSE);
:
:
	{ &hf_gsm_a_rr_multirate_speech_ver,
		{ "Multirate speech
version","gsm_a.rr.multirate_speech_ver",
		FT_UINT8,BASE_DEC,  VALS(multirate_speech_ver_vals), 0xe0,

		"Multirate speech version", HFILL }
	},
	{ &hf_gsm_a_rr_NCSB,
		{ "NSCB: Noise Suppression Control Bit","gsm_a.rr.NCSB",
		FT_UINT8,BASE_DEC,  VALS(NSCB_vals), 0x10,          
		"NSCB: Noise Suppression Control Bit", HFILL }
	},
	{ &hf_gsm_a_rr_ICMI,
		{ "ICMI: Initial Codec Mode Indicator","gsm_a.rr.ICMI",
		FT_UINT8,BASE_DEC,  VALS(ICMI_vals), 0x8,          
		"ICMI: Initial Codec Mode Indicator", HFILL }
	},
	{ &hf_gsm_a_rr_set_of_amr_codec_modes_v1_b7,
      { "10,2 kbit/s codec rate", "gsm_a.rr.set_of_amr_codec_modes_v1b7",
		FT_BOOLEAN,8,  TFS(&gsm_a_rr_set_of_amr_codec_modes), 0x40,

		"10,2 kbit/s codec rate", HFILL }
	},
The "0x40" value is the mask value.
Brg
Anders 

-----Ursprungligt meddelande-----
Från: ethereal-dev-bounces@xxxxxxxxxxxx
[mailto:ethereal-dev-bounces@xxxxxxxxxxxx] För Piros Lucian
Skickat: den 1 september 2005 14:59
Till: ronnie sahlberg
Kopia: Ethereal development
Ämne: [Ethereal-dev] Re: cimd dissector

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>

_______________________________________________
Ethereal-dev mailing list
Ethereal-dev@xxxxxxxxxxxx
http://www.ethereal.com/mailman/listinfo/ethereal-dev