[Click] DS: New element: icmpipencap, a general icmppingencap

[Jens De Wit]

Hi Eddie,

I'm sending you an update of the ICMPIPEncap element that addresses your
previous remarks. That click_icmp_hl() function was quite useful indeed. The
other 2 issues were solved simultaneously by simply initializing the
complete icmp header to 0 at the start. I've also updated the comments in
the header file a bit.
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.

Best Regards,
Jens


2010/3/4 Eddie Kohler <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>
>>
>>  -------------------------------------------
>>> *Van:* Eddie Kohler[SMTP:KOHLER at CS.UCLA.EDU <SMTP%3AKOHLER at CS.UCLA.EDU><
>>> SMTP%3AKOHLER at CS.UCLA.EDU <SMTP%253AKOHLER at CS.UCLA.EDU>>]
>>> *Verzonden:* zondag 28 februari 2010 21:52:07
>>> *Aan:* Braem Bart
>>> *CC:* 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
>>>> https://amsterdam.lcs.mit.edu/mailman/listinfo/click
>>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> _______________________________________________
>>> click mailing list
>>> click at amsterdam.lcs.mit.edu
>>> https://amsterdam.lcs.mit.edu/mailman/listinfo/click
>>>
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: icmpipencap.cc
Type: text/x-c++src
Size: 3915 bytes
Desc: not available
Url : http://amsterdam.lcs.mit.edu/pipermail/click/attachments/20100309/32685761/attachment-0001.cc 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: icmpipencap.hh
Type: text/x-c++hdr
Size: 1796 bytes
Desc: not available
Url : http://amsterdam.lcs.mit.edu/pipermail/click/attachments/20100309/32685761/attachment-0001.hh