Mailing List Archive

Re: Kismet protocol dissector
Hi,

Some comments

1, please create a WIKI page on wiki.wireshark.org for this protocol.
Fill it out as much as possible to make it as nice and informative as
possible for causal non-kismet expert users

2, upload the example traces to that wiki page.

3, you use "hidden" fields such as the hf_kismet_[request|response]
Hidden fields are in general a bad idea since it prevents the user
from knowing about them.
Please redo these as "generated" fields instead so they are visible
in the trace and the users will find out about them and can use them.
Please grep for PROTO_ITEM_SET_GENERATED for examples on how to
display "generated" fields as "generated".
Note: it often makes sense to try to put all "generated" fields at the
top of the expansion where they reside.

4, you usae tvb_get_string whis requires the data to ge g_free()d
which you do! but it would be better to switch to something like
tvb_get_ephemeral_string() that does not require an explicit free
and thus would not leak memory if an exception is caused between teh
tvb_get_string() andf the g_free() occurs.


5, is it possible to change the dissector to be a new style
dissector returning gboolean instead of an oldstyle returning void?
please see mailinglist archive for reasons why new-style is better
than oldstyle.



please address these issues and resubmit the patch to the mailinglist
for rereview.


best regards
ronnie s




On 7/12/06, Krzysztof Burghardt <krzysztof@burghardt.pl> wrote:
> Hello,
>
> I made Kismet protocol dissector for Ethereal for Client/Server protocol.
> Kismet Drone/Server protocol still needs separate dissector, because it
> uses different protocol.
>
> Patch for current SVN revision (e.g. 18189) attached.
>
> More information, including protocol traces can be found here:
> http://www.burghardt.pl/wiki/software/kismet_protocol_dissector_for_ethereal
>
> Regards,
> --
> Krzysztof Burghardt <krzysztof@burghardt.pl>
> http://www.burghardt.pl/
>
>
_______________________________________________
Ethereal-dev mailing list
Ethereal-dev@ethereal.com
http://www.ethereal.com/mailman/listinfo/ethereal-dev
Re: Kismet protocol dissector [ In reply to ]
Thanks for your reply.

> 1, please create a WIKI page on wiki.wireshark.org for this protocol.

Done, see it here:
http://wiki.wireshark.org/Kismet_Client/Server_Protocol
http://wiki.ethereal.com/Kismet_Client/Server_Protocol

> 2, upload the example traces to that wiki page.

done

> 3, you use "hidden" fields such as the hf_kismet_[request|response]
> Hidden fields are in general a bad idea since it prevents the user
> from knowing about them.

Changed to "generated" fields.

> 4, you usae tvb_get_string whis requires the data to ge g_free()d

Changed to use tvb_get_ephemeral_string().

> 5, is it possible to change the dissector to be a new style
> dissector returning gboolean instead of an oldstyle returning void?

Here is a (small) problem. I changed dissector, so it use gboolean and
always return true, but I have no idea how to check if it is kismet or
not. Some suggestions?

> please address these issues and resubmit the patch to the mailinglist
> for rereview.

Patch attached.

Regards,
--
Krzysztof Burghardt <krzysztof@burghardt.pl>
http://www.burghardt.pl/
Re: Re: Kismet protocol dissector [ In reply to ]
On 7/13/06, Krzysztof Burghardt <krzysztof@burghardt.pl> wrote:
>
> Thanks for your reply.
>
> > 1, please create a WIKI page on wiki.wireshark.org for this protocol.
>
> Done, see it here:
> http://wiki.wireshark.org/Kismet_Client/Server_Protocol
> http://wiki.ethereal.com/Kismet_Client/Server_Protocol


Nice!

However, port 2501 is registered for the rtsclient protocol and unless
kismet is the same as rtsclient
it would be incorrect to refer to this as a well-known port for kismet.


further down on the page the port is referred to as the default port?


if it is a default port you should add the port as a preference setting
which defaults to 2501 but can be changed by the user.



> 5, is it possible to change the dissector to be a new style
> > dissector returning gboolean instead of an oldstyle returning void?
>
> Here is a (small) problem. I changed dissector, so it use gboolean and
> always return true, but I have no idea how to check if it is kismet or
> not. Some suggestions?




Kismet is an ASCII based protocol?
If so you may check that the first 8 bytes of the tvb (if there are 8 bytes
or more) are actual ascii values >32 <128 ?


You have a lot of
offset += next_token - line; linelen -= next_token - line; line =
next_token;
can you break these up to one assignment/statement per row and add a blank
line after each proto_tree_add_text() to separate the dissection block for
each field out from eachothers
Re: Kismet protocol dissector [ In reply to ]
> However, port 2501 is registered for the rtsclient protocol and unless
> kismet is the same as rtsclient
> it would be incorrect to refer to this as a well-known port for kismet.
> further down on the page the port is referred to as the default port?

So this is probably not a registered port. I think it was assigned by
Kismet's author.

> if it is a default port you should add the port as a preference setting
> which defaults to 2501 but can be changed by the user.

Yes, users can change port. I made it a preference.

> Kismet is an ASCII based protocol?

Yes, it is.

> If so you may check that the first 8 bytes of the tvb (if there are 8
> bytes or more) are actual ascii values >32 <128 ?

Done.

> You have a lot of
> offset += next_token - line; linelen -= next_token - line; line =
next_token;
> can you break these up to one assignment/statement per row and add a
blank

All done. Patch attached.
Regards,
--
Krzysztof Burghardt <krzysztof@burghardt.pl>
http://www.burghardt.pl/
Re: Kismet protocol dissector [ In reply to ]
> wiki.wireshark.org/SampleCaptures and then link to these captures
[...]
> can you move them to /SampleCaptures and link there from the protocol page

Sure, done.

> You put the braces for compound statement differently from the rest of
wireshark coding style
> which (most of the time/often) is
> if (expression) {

Ups, I did it again ;->
It's fixed now (indent -gnu -br). Patch attached.

> Ports only become WellKnown if it is IANA that assigns them.

Right, fixed this in wiki.

Regards,
--
Krzysztof Burghardt <krzysztof@burghardt.pl>
http://www.burghardt.pl/