checked in
On 5/14/05, Guillaume Chazarain <guichaz@xxxxxxxx> wrote:
> >
> >
> >1,
> >dont use armagetronad_descriptor, use
> >a value_string as all other dissectors do.
> >That will remove the need for get_descriptor() as well since you would
> >use val_to_str() instead.
> >
> >
> OK, Done. Anyway, it was a premature optimization, so not much is lost.
>
> >2,
> >add_message_data() : rewrite it completely.
> >
> >
> I don't understand the motivation for this one.
>
> >Do not use tvb_memdup(), do not read directly from this buffer.
> >
> >
> I thought tvb_memdup() was specifically created so as to read the packet
> data
> directly from a pointer without worrying about the packet being too short.
> And given the maximum size of a UDP packet I was not afraid of using
> too much memory. Confused ...
>
> >IF the fields are of type little-endian 16 bit integers, read them
> >one by one using tvb_get_letohs()
> >
> >
> Here things start to complicate. This 'Data' field as its name implies
> has no precise syntax. Typically it contains 16 bit integers displayed
> perfectly in ethereal hex dump, but sometimes it contains a string with
> bytes swapped (don't ask me why) so I swap them to make the string
> readable. So basically, it's a char* with bytes swapped, and I was quite
> happy with my handling ...
>
> >3,
> >dissect_armagetronad()
> >change this dissector to become a "new" style dissector i.e. first
> >doing some heuristics and testing if it really is armagetroinad or not
> >and return 0 if not.
> >see packet-snmp.c for how this is done for snmp over udp.
> >
> >
> OK, done. Hopefully I got it right.
>
> >4,
> >Do you really need to specify it as a heuristic dissector as well?
> >
> >
> No I don't really need it.
>
> >If it uses a fixed port normally, just leave it as a normal dissector
> >
> >
> Yes the port is usually fixed.
>
> >attached to a port, then if the user wants to, he can always do a
> >DecodeAs and specify a different non-standard port for it.
> >
> >
> I was targetting a different use case : an admin wants to know what the
> network is used for. Ethereal tells him people play Armagetronad trying
> to hide using a non-default port ;-) Another aspect is that it was easy
> to do. Anyway, I removed it since you seem to imply that it must be done
> only when it's needed because the port number is not well known.
>
> Thanks for the review.
>
> Regards.
>
> --
> Guillaume
>
>
>