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

[Eddie Kohler]

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>]
>> *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