Sure ... Makes sense.
I will modify it, and since practically the complete patch is changed, I
will sent out another patch.
Thanks!
Mehak
On Wed, Aug 1, 2012 at 12:57 PM, Ethan Jackson <et...@nicira.com> wrote:
> > I agree its cleaner, but I am still inclined to keep it as is because
> sizeof
> > *ip does not contain the options which should also be included in
> computing
> > the IP checksum. IP_IHL takes the header length and uses that which is
> what
> > the IP Checksum calculations should use. What do you think ?
>
> Well, this function doesn't use options, but let's suppose for a
> second that it did. It feels very strange to me that we would be
> telling a function (csum()) to operate beyond the end of a structure.
> I.E. sizeof *ip == 20 but we're telling csum to operate on 24 bytes of
> *ip. That doesn't feel like good memory management to me. If there
> were options I would be inclined to calculate the checksum of the
> struct ip_header first, and then fold in the checksum of the options
> as a second step.
>
> Luckily this function doesn't use options so simply doing the first
> step seems sufficient to me.
>
> Does that reasoning sound reasonable?
>
> Also, I noticed a bug in this function which existed before this
> patch. I'll send out a fix.
>
> Ethan
>
>
> >
> > thanx!
> > mehak
> >
> >
> > On Wed, Aug 1, 2012 at 12:05 PM, Ethan Jackson <et...@nicira.com> wrote:
> >>
> >> > + ip->ip_csum = csum(ip, IP_IHL(ip->ip_ihl_ver) * 4);
> >>
> >> I think what you have here is correct, but would be a bit
> >> safer/cleaner if we changed it to:
> >>
> >> ip->ip_csum = csum(ip, sizeof *ip);
> >>
> >> Ethan
> >>
> >>
> >> > } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
> >> > /* XXX */
> >> > } else if (flow->dl_type == htons(ETH_TYPE_ARP)) {
> >> > --
> >> > 1.7.2.5
> >> >
> >> > _______________________________________________
> >> > dev mailing list
> >> > dev@openvswitch.org
> >> > http://openvswitch.org/mailman/listinfo/dev
> >
> >
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
