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>
>
>
