in_cksum() for ip packets with multiple mbufs

[kamal kc]

> > void copy_the_memorybuffer(struct mbuf **m)
> > {
> >    struct mbuf *mbuf_pointer=*m;
> >    struct mbuf **next_packet;
> >
> >    next_packet=&mbuf_pointer;
> >
> >    struct ip *my_ip_hdr;
> >    my_ip_hdr=mtod((*next_packet),struct ip *);
> >    my_ip_hdr->ip_tos=64;
> >    my_ip_hdr->ip_sum=0;
> >
> >    my_ip_hdr->ip_sum=
> >       
> in_cksum((*next_packet),(my_ip_hdr->ip_hl<<2));
> >   .......
> > }
> 
> Why all this pointer fun?  How do you know that it's
> safe to dereference
> `m' when you do:
> 
> 	struct mbuf *mbuf_pointer=*m;
> 
> Why are you dereferencing `m' once to obtain
> mbuf_pointer and then
> taking the address of that to obtain next_packet,
> when you could
> just do:
> 
> 	next_packet = m;
> 
> There are also several other problems with
> copy_the_memorybuffer() as
> it's written above:
> 
>     - It's considered bad style to mix declarations
> and code the way you
>       have done above
> 
>     - It is probably better to return the copy of
> the mbuf you're
>       fiddling with, instead of modifying in place a
> parameter of the
>       function.
 
one thing i would like to ask?

does it make any difference if i free the mbuf 'm'
passed to if_output() and pass my own mbuf to 
if_output. 

is the original mbuf referenced by any
other pointers or global variables ?? 

i couldn't figure out much from the sources.
 
>     - If you are not *REALLY* copying the data of
> the mbuf, then
>       the name of copy_the_memorybuffer() is very
> confusing.

i didn't showed in the above code snippet but
actually i am copying the data contained in the 
mbufs in a character array. 

my purpose is to compress the data contained in
the ip packet
 
>     - What is the magic constant 64 that is assigned
> to ip_tos?

that was just to see that i could actually modify
the ip header.

> What you probably want to do is something like:
> 
>     void
>     ip_set_type_of_service(struct mbuf *m)
>     {
>         struct ip *iph;
>     
>         assert(m != NULL);
>     
>         iph = mtod(m, struct ip *);
>         iph->ip_tos = IPTOS_PREC_IMMEDIATE;
>         iph->ip_sum = 0;
>         iph->ip_sum = in_cksum((uint16_t *)iph,
> iph->ip_hl << 2);
>     }

thanks i will try this code and try to make the 
code simpler next time.

> but that's not copying anything.
> 
> > but still it doesn't seem to work. and the problem
> > is still there.
> 
> You have obviously made a lot of changes that we
> haven't seen yet.

i did not put the whole code because i am a bit lazy 
and the code is cumbersome. and i thought that 
it was causing the problem.

> Instead of posting snippets here and there, save a
> copy of the original
> sources somewhere, then a copy of the new sources,
> and run diff(1) on
> the two directories to extract *ALL* the changes.
> 
> 	$ cd /usr/src
> 	$ diff -ruN src.old/ src/ > /tmp/patchfile
> 
> and put the patchfile somewhere online where we can
> have a look at all
> the changes.

i am new to kernel sources and kernel programming and
thank you for informing me on the diff(1).

thanks to you that the problem was solved (i don't 
know if it is completely ok). i found that
   - i had made mistake in computing checksum. 
      earlier checksum was computed over the whole 
      data but now i calculate checksum over the 
      header only.
    - byte ordering. 
    tell me one thing isn't the byte order of the 
 ip_id in network byte order in the mbuf when passed 
  to the if_output() ??
  i had to use the htons() to ip_id before computing 
  the checksum
   -  final thing does this makes any difference
     (calling the htons() twice):

      ip->ip_id=htons(ip->ip_id);
      ip->ip_id=htons(ip->ip_id);

I will try put up patchfiles on web. 

thanks very much,
kamal




 



	
		
__________________________________ 
Yahoo! Mail - PC Magazine Editors' Choice 2005 
http://mail.yahoo.com