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 <[email protected]>
> 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 <[email protected]>
>>
>> -------------------------------------------
>>> *Van:* Eddie Kohler[SMTP:[email protected] <smtp%[email protected]><
>>> smtp%[email protected] <smtp%[email protected]>>]
>>> *Verzonden:* zondag 28 februari 2010 21:52:07
>>> *Aan:* Braem Bart
>>> *CC:* [email protected]; 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
>>>> [email protected]
>>>> https://amsterdam.lcs.mit.edu/mailman/listinfo/click
>>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> _______________________________________________
>>> click mailing list
>>> [email protected]
>>> https://amsterdam.lcs.mit.edu/mailman/listinfo/click
>>>
>>
#include <click/config.h>
#include "icmpipencap.hh"
#include <click/confparse.hh>
#include <click/error.hh>
#include <clicknet/ip.h>
#include <clicknet/icmp.h>
#include <click/packet_anno.hh>
#include <click/nameinfo.hh>
#include <click/standard/alignmentinfo.hh>
CLICK_DECLS
ICMPIPEncap::ICMPIPEncap()
: _icmp_id(0), _ip_id(1), _icmp_type(0), _icmp_code(0)
{
}
ICMPIPEncap::~ICMPIPEncap()
{
}
int
ICMPIPEncap::configure(Vector<String> &conf, ErrorHandler *errh)
{
String code_str = "0";
int code;
if (cp_va_kparse(conf, this, errh,
"SRC", cpkP+cpkM, cpIPAddress, &_src,
"DST", cpkP+cpkM, cpIPAddress, &_dst,
"TYPE", cpkP+cpkM, cpNamedInteger, NameInfo::T_ICMP_TYPE, &_icmp_type,
"CODE", cpkP, cpWord, &code_str,
"IDENTIFIER", 0, cpUnsignedShort, &_icmp_id,
cpEnd) < 0)
return -1;
if (!NameInfo::query_int(NameInfo::T_ICMP_CODE + _icmp_type, this, code_str, &code) || code < 0 || code > 255)
return errh->error("invalid code");
_icmp_code = code;
#if HAVE_FAST_CHECKSUM && FAST_CHECKSUM_ALIGNED
{ // check alignment
int ans, c, o;
ans = AlignmentInfo::query(this, 0, c, o);
_aligned = (ans && c == 4 && o == 0);
if (!_aligned)
errh->warning("IP header unaligned, cannot use fast IP checksum");
if (!ans)
errh->message("(Try passing the configuration through `click-align'.)");
}
#endif
return 0;
}
Packet *
ICMPIPEncap::simple_action(Packet *p)
{
if (WritablePacket *q = p->push(sizeof(click_ip) + click_icmp_hl(_icmp_type))) {
click_ip *ip = reinterpret_cast<click_ip *>(q->data());
ip->ip_v = 4;
ip->ip_hl = sizeof(click_ip) >> 2;
ip->ip_tos = 0;
ip->ip_len = htons(q->length());
ip->ip_id = htons(_ip_id);
ip->ip_off = 0;
ip->ip_ttl = 255;
ip->ip_p = IP_PROTO_ICMP; /* icmp */
ip->ip_sum = 0;
ip->ip_src = _src;
ip->ip_dst = _dst;
click_icmp_sequenced *icmp = (struct click_icmp_sequenced *) (ip + 1);
memset(icmp, 0, click_icmp_hl(_icmp_type));
icmp->icmp_type = _icmp_type;
icmp->icmp_code = _icmp_code;
if(_icmp_type == ICMP_TSTAMP || _icmp_type == ICMP_TSTAMPREPLY || _icmp_type == ICMP_ECHO
|| _icmp_type == ICMP_ECHOREPLY || _icmp_type == ICMP_IREQ || _icmp_type == ICMP_IREQREPLY) {
#ifdef __linux__
icmp->icmp_identifier = _icmp_id;
icmp->icmp_sequence = _ip_id;
#else
icmp->icmp_identifier = htons(_icmp_id);
icmp->icmp_sequence = htons(_ip_id);
#endif
}
#if HAVE_FAST_CHECKSUM && FAST_CHECKSUM_ALIGNED
if (_aligned)
ip->ip_sum = ip_fast_csum((unsigned char *)ip, sizeof(click_ip) >> 2);
else
ip->ip_sum = click_in_cksum((unsigned char *)ip, sizeof(click_ip));
#elif HAVE_FAST_CHECKSUM
ip->ip_sum = ip_fast_csum((unsigned char *)ip, sizeof(click_ip) >> 2);
#else
ip->ip_sum = click_in_cksum((unsigned char *)ip, sizeof(click_ip));
#endif
icmp->icmp_cksum = click_in_cksum((const unsigned char *)icmp, q->length() - sizeof(click_ip));
q->set_dst_ip_anno(IPAddress(_dst));
q->set_ip_header(ip, sizeof(click_ip));
_ip_id += (_ip_id == 0xFFFF ? 2 : 1);
return q;
} else
return 0;
}
String ICMPIPEncap::read_handler(Element *e, void *thunk)
{
ICMPIPEncap *i = static_cast<ICMPIPEncap *>(e);
if (thunk)
return IPAddress(i->_dst).unparse();
else
return IPAddress(i->_src).unparse();
}
int ICMPIPEncap::write_handler(const String &str, Element *e, void *thunk, ErrorHandler *errh)
{
ICMPIPEncap *i = static_cast<ICMPIPEncap *>(e);
IPAddress a;
if (!cp_ip_address(str, &a))
return errh->error("expected IP address");
if (thunk)
i->_dst = a;
else
i->_src = a;
return 0;
}
void ICMPIPEncap::add_handlers()
{
add_read_handler("src", read_handler, (void *) 0, Handler::CALM);
add_write_handler("src", write_handler, (void *) 0);
add_read_handler("dst", read_handler, (void *) 1, Handler::CALM);
add_write_handler("dst", write_handler, (void *) 1);
}
CLICK_ENDDECLS
EXPORT_ELEMENT(ICMPIPEncap)
#ifndef CLICK_ICMPIPENCAP_HH
#define CLICK_ICMPIPENCAP_HH
#include <click/element.hh>
#include <click/timer.hh>
CLICK_DECLS
/*
=c
ICMPIPEncap(SRC, DST, TYPE [, CODE, I<keywords> IDENTIFIER])
=s icmp
encapsulates packets in ICMP/IP headers
=d
Encapsulates input packets in an ICMP/IP header with source IP address SRC,
destination IP address DST, ICMP type TYPE and code CODE. TYPE and CODE
may be integers between 0 and 255 or mnemonic names; CODE defaults to 0.
If the corresponding header of the specified ICMP type contains a sequence
field, it is advanced by one for each packet. (The sequence field is
stored in network byte order in the packet.)
Keyword arguments are:
=over 8
=item IDENTIFIER
Integer (also stored in network byte order). Determines the ICMP identifier
field in emitted ICMP packets. Default is 0.
=back
=h src read/write
Returns or sets the SRC argument.
=h dst read/write
Returns or sets the DST argument.
=a
ICMPPingEncap, ICMPError */
class ICMPIPEncap : public Element { public:
ICMPIPEncap();
~ICMPIPEncap();
const char *class_name() const { return "ICMPIPEncap"; }
const char *port_count() const { return PORTS_1_1; }
const char *processing() const { return AGNOSTIC; }
const char *flags() const { return "A"; }
int configure(Vector<String> &, ErrorHandler *);
void add_handlers();
Packet *simple_action(Packet *);
private:
struct in_addr _src;
struct in_addr _dst;
uint16_t _icmp_id;
uint16_t _ip_id;
uint8_t _icmp_type;
uint8_t _icmp_code;
#if HAVE_FAST_CHECKSUM && FAST_CHECKSUM_ALIGNED
bool _aligned;
#endif
static String read_handler(Element *, void *);
static int write_handler(const String &, Element *, void *, ErrorHandler *);
};
CLICK_ENDDECLS
#endif
_______________________________________________
click mailing list
[email protected]
https://amsterdam.lcs.mit.edu/mailman/listinfo/click
