Problem with zcip's packing of ARP packets?

[Jason Schoon]

On 8/25/06, Patrick Noffke <Patrick.Noffke at adpro.com.au> wrote:
>
> > -----Original Message-----
> > From: busybox-bounces at busybox.net
> > [mailto:busybox-bounces at busybox.net]On
> > Behalf Of Rich Felker
> > Sent: Friday, 25 August 2006 3:21 PM
> > To: busybox at busybox.net
> > Subject: Re: Problem with zcip's packing of ARP packets?
> >
> >
> > On Thu, Aug 24, 2006 at 09:55:08PM -0700, Andre wrote:
> > > --- Jason Schoon <floydpink at gmail.com> wrote:
> > > >
> > > > I have seen this problem as well, and submitted a patch
> > to the list
> > > > some time ago.  I don't believe it ever got applied though.
> > > > Here it is again.
> > >
> > > Patch looks good except for this:
> > >
> > > > +   /* This cast is safe because ip is always kept as BE */
> > > > +   if ((*(unsigned long *)p.arp.arp_spa == ip.s_addr)
> > >
> > > The cast is NOT safe (but not for the reason you mention... ;-).
> > >
> > > p is a packed structure, therefore arp_spa is not necessarily 32bit
> > > aligned... which means it can't be accessed via an unsigned long
> > > pointer on architectures which care about alignment...
> >
> > It's not being accessed as an unsigned long pointer. It's being
> > accessed as whatever type the arp_spa member is, and then _cast_ to a
> > pointer to unsigned long. However the code looks wrong. I suspect it
> > was intended to do what you said, i.e. the intent was:
> >
> > if ((*(unsigned long *)&p.arp.arp_spa == ip.s_addr)
> >
> > and this of course is invalid if arp_spa is not aligned. Like you say,
> > memcmp would be the correct approach.
> >
>
> I have updated my patch with the state machine implementation according to
> the above advice.  New patch is against 1.2.1 and is attached.  I'll
> update issue 1005 as well.
>
> Pat


Pat,

Do you have a size comparison of your state machine implementation vs. the
current code.  I would like to see the state machine integrated, since the
code is much simpler to understand in my opinion.  If it just so happened
that it was smaller also, I think that would be even more convincing.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.busybox.net/pipermail/busybox/attachments/20060825/abed1c91/attachment.htm