Wireshark-dev: Re: [Wireshark-dev] [PATCH] New Dissector : Roofnet
From: Jaap Keuter <[email protected]>
Date: Tue, 19 Dec 2006 18:22:10 +0100 (CET)
Hi, Sebastien,

On Tue, 19 Dec 2006, Sebastien Tandel wrote:

> Hi Jaap,
>
>
> > * I'm not sure about using get_hostname here. Shouldn't that be handled
> > through Wireshark services.
> >
> What do you mean? get_hostname is already part of the wireshark API
> (addr_resolv) and is used in many dissectors. Should I use another function?

No, you're absolutely right. I was called into a meeting and just posted
the question. Keep it in there, it's correct.

> > * The loop in dissect_roofnet should check that it doesn't spin out of
> > control when an incorrectly large value is read.
> >
> The roofnet length is restricted to 400 bytes (maybe 200 in a near
> future). I then control whetherr the length of the announced number of
> links is greater than this max length (400). If it's the case I print an
> error in the tree, add an expert info value and stop the dissection of
> the packet. Is it sufficient?

I rather would like a check against the actual size of the packet, to
avoid going out of bounds at tvb access.

> > * Use %u i.s.o. %d when printing unsigned values
> done

Great.

> > * Don't put a comma after the last entry in a structure array initializer
> >
> I'm ok with that (and it's done) but do you know there are around 4100
> structures initialized like that only in the dissectors part? :-/

True, and I don't like it, it's sloppy. So I'm gonna pound on them until
they're gone. Thanks for taking them out.

> > * Use the 'standard' file header as found in the README.developer
> >
> Did you mean stdio, stdlib ? If not, give me a hint 'cause I don't see ...

I mean the copyright stuff, like

/* packet-PROTOABBREV.c
 * Routines for PROTONAME dissection
 * Copyright 200x, YOUR_NAME <YOUR_EMAIL_ADDRESS>
 *
 * $Id$
 *
 * Wireshark - Network traffic analyzer
 * By Gerald Combs <[email protected]>
 * Copyright 1998 Gerald Combs
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.

etc.

Furhter question: did you fuzz test this dissector on some real life
roofnet captures?

Thanx,
Jaap