Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 23 Oct 2005 15:04:42 +0300
From:      Giorgos Keramidas <keramida@ceid.upatras.gr>
To:        kamal kc <kamal_ckk@yahoo.com>
Cc:        freebsd <freebsd-hackers@freebsd.org>, freebsd <freebsd-net@freebsd.org>
Subject:   Re: in_cksum() for ip packets with multiple mbufs
Message-ID:  <20051023120442.GB29924@flame.pc>
In-Reply-To: <20051023111333.53650.qmail@web35701.mail.mud.yahoo.com>
References:  <20051023101919.GA9957@flame.pc> <20051023111333.53650.qmail@web35701.mail.mud.yahoo.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On 2005-10-23 04:13, kamal kc <kamal_ckk@yahoo.com> wrote:
> /* the argument m is the (struct mbuf *) that
>  * contains the packet data
>  */
>
> 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.

    - If you are not *REALLY* copying the data of the mbuf, then
      the name of copy_the_memorybuffer() is very confusing.

    - What is the magic constant 64 that is assigned to ip_tos?

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);
    }

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.
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.



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20051023120442.GB29924>