[Click] DS: New element: icmpipencap, a general icmppingencap
Eddie Kohler
kohler at cs.ucla.edu
Tue Mar 9 12:38:36 EST 2010
Hi Jens,
Excellent! The patch is applied.
Jens De Wit wrote:
> There's one thing I'm a bit curious about though: I've noticed the byte
> orderings of the sequence number and the ICMP identifier are in network
> order (just like in the ICMPPingEncap element on which Nico and me
> originally based this new element). I was wondering if there is any
> specific reason why this was done? All other fields seem to follow host
> ordering, including for instance the IP identifier.
Well, some of the fields have no ordering. ICMP type and ICMP code are both
one byte long -- no order applies. The ICMP identifier and ICMP ID fields are
sent in network order by most BSD-derived stacks [i.e., htons() is applied],
but in host order by Linux -- thus the #ifdef __linux__. Make sense? The IP
ID is sent in network order in the current code, but this doesn't really
matter; host order would be fine, since other hosts on the network are
supposed to treat IP IDs like opaque tokens, not sequence numbers. (We only
care about the ordering so that IPPrint/tcpdump will print the expected values.)
Eddie
>
> Best Regards,
> Jens
>
>
> 2010/3/4 Eddie Kohler <kohler at cs.ucla.edu <mailto:kohler at cs.ucla.edu>>
>
> Jens,
>
> Excellent! Thank you so much for this. The new element is added.
>
> I do have some remaining comments, so if you're willing you might
> consider sending another patch:
>
> * You may be able to simplify the "switch (_icmp_type)" using the
> "click_icmp_hl()" function, which returns the ICMP header length
> appropriate for a given ICMP type. This function is defined in
> <click/icmp.h>. You would still need to set "need_seq_number" based
> on _icmp_type.
>
> * The TSTAMP and TSTAMPREPLY ICMP types contain more data than the
> other types. This data area is currently left uninitialized, so it
> contains garbage. This is OK, but it might be less surprising to
> initialize this area to zero. Something like this would do it:
>
> memset(static_cast<uint8_t *>(icmp) + sizeof(click_icmp), 0,
> click_icmp_hl(_type) - sizeof(click_icmp));
>
> * The "identifier" field is only relevant i f"need_seq_number" is
> true. If "need_seq_number" is false, I would instead initialize the
> identifier and sequence number to 0 (this is like initializing the
> "padding" field to 0).
>
> Thanks again.
> Eddie
>
>
>
> Jens De Wit wrote:
>
> Dear list,
>
> I send this mail in reply to Eddie's remarks on the new
> ICMPIPEncap element.
> Attached you will find the improved version of the element, as
> well as a
> testie file containing a basic unit test for the new element.
>
> Kind regards,
> Jens De Wit
>
>
>
> 2010/2/28 De Wit Jens <Jens.DeWit at student.ua.ac.be
> <mailto:Jens.DeWit at student.ua.ac.be>>
>
> -------------------------------------------
> *Van:* Eddie Kohler[SMTP:KOHLER at CS.UCLA.EDU
> <mailto:SMTP%3AKOHLER at CS.UCLA.EDU>
> <SMTP%3AKOHLER at CS.UCLA.EDU
> <mailto:SMTP%253AKOHLER at CS.UCLA.EDU>>]
> *Verzonden:* zondag 28 februari 2010 21:52:07
> *Aan:* Braem Bart
> *CC:* click at amsterdam.lcs.mit.edu
> <mailto:click at amsterdam.lcs.mit.edu>; De Wit Jens
> *Onderwerp:* Re: [Click] New element: icmpipencap, a general
> icmppingencap
>
> *Automatisch doorgezonden volgens een regel*
>
> Hi guys!
>
> Thanks very much for this element. I'd definitely like to
> add it to the
> repository. However, I have some concerns that if you could
> address, it
> would
> be great.
>
> 1. Element documentation in the header file is missing.
> Even just one
> paragraph plus the use case (e.g. "=c ICMPEncap(SRC, DST,
> TYPE [, CODE])".)
>
> 2. The _icmp_type and _icmp_code members should be read
> using IPNameInfo,
> so
> users can give symbolic names. E.g.:
>
> int32_t icmp_type;
> ... "TYPE", cpkP+cpkM, cpNamedInteger,
> NameInfo::T_ICMP_TYPE, &icmp_type,
> ...
>
> The CODE requires special handling since you need to know
> the TYPE before
> parsing the CODE. Take a look at ICMPError for an example.
>
> 3. This is the big one: The current element adds a
> clcik_icmp_echo header.
> But this header is only correct for TYPE echo or echo-reply.
> Other ICMP
> types
> add a different header, often with a different length. For
> example tstamp
> and
> tcstamp-reply have longer ehaders. See
> include/clicknet/icmp.h for more
> examples. Either the element should be changed to add the
> appropriate
> header
> for _type, or the configure() method should return an error
> on an
> inappropriate TYPE.
>
> Thanks very much,
> Eddie
>
>
> Braem Bart wrote:
>
> Dear list,
>
> Attached you will find an element our students (Jens De
> Wit and Nico Van
>
> Looy) produced while working on their FireSim project, the
> Firewall
> Simulation in Click as presented on SyClick.
>
> The element allows transmitting ICMP packets with all
> types of codes and
>
> types, which makes this element more general than
> ICMPPingEncap. It is a bit
> the ICMP alternative to UDPIPEncap.
>
> The element code is largely based on ICMPPingEncap so
> Jens and Nico
>
> suggested just patching ICMPPingEncap with the new
> behaviour, although of
> course the name then does not cover the contents of the element.
>
> Jens promised to write a unit test with the testie
> framework, so this can
>
> be expected as well.
>
> best regards,
> Bart
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> click mailing list
> click at amsterdam.lcs.mit.edu
> <mailto:click at amsterdam.lcs.mit.edu>
> https://amsterdam.lcs.mit.edu/mailman/listinfo/click
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> click mailing list
> click at amsterdam.lcs.mit.edu <mailto:click at amsterdam.lcs.mit.edu>
> https://amsterdam.lcs.mit.edu/mailman/listinfo/click
>
>
More information about the click
mailing list