[Click] set_ip_header semantics

[Eddie Kohler]

(But maybe that argument just isn't a big deal, and Packet::ip_header() and 
Packet::set_ip_header_anno() can usefully coexist.)

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