[Click] set_ip_header semantics
Eddie Kohler
kohler at cs.ucla.edu
Tue Nov 4 11:14:08 EST 2008
Bart,
I'm interested to look at the patch. But note that for consistency,
Packet::set_ip_header_anno() would probably imply that that one accesses the
IP header via Packet::ip_header_anno(), and so forth. We currently use
Packet::ip_header(), which is better....
Eddie
Bart Braem wrote:
> Hi Eddie,
>
> On 31 Oct 2008, at 01:37, Eddie Kohler wrote:
>
>> Bart Braem wrote:
>>> Students were doing exercise where they had to change an IP header
>>> of a packet. What they did was use ip_header to get the current
>>> ip_header, take a copy of that struct, modify it and then set it with
>>> set_ip_header. And that did not work.
>>> We were also confused in the beginning, until we realised the
>>> semantics of set_ip_header: it only sets the pointers and it does
>>> not copy anything. In fact, do we understand correctly that the
>>> set_ip_header should be treated as an annotation function and that
>>> the pointer set with it should lie within the packet boundaries?
>>
>> That's exactly correct. In fact, since commit
>> f03d536cc8392f673fb90da298ebbacb77eb3ca1, we assert that the pointer
>> is in range.
>>
>
> I did not notice that patch, the assertion certainly makes sense there.
> Thanks!
>
>
>>> We suggest to update the network header methods to better reflect
>>> these semantics.
>>> We could introduce naming like set_ip_header_anno.
>>> But we also suggest to change the call itself and use an offset
>>> parameter instead of a pointer to the data. This way confusion is
>>> impossible because it is clear that the network_header methods work
>>> on in-packet data.
>>> What do you think?
>>
>> I see what you mean, but don't you think the assertion is enough?
>> Perhaps a name change to _anno() would be useful, but set_ip_header()
>> is very well established by this point. My tendency is to leave
>> things as they are.
>
>
> In my opinion a name change would be better, but I do understand that
> then also similar functions for all other headers would have to be
> changed as well to stay consistent. Which is quite a large job, but I am
> willing to create the patch myself.
> It will be a large job, but it can only improve the internal consistency
> and the accessibility of Click to new users.
>
> Regards,
> Bart
More information about the click
mailing list