From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1NBBUz-0002pR-3z
	for qemu-devel@nongnu.org; Thu, 19 Nov 2009 13:12:53 -0500
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1NBBUt-0002iT-Uf
	for qemu-devel@nongnu.org; Thu, 19 Nov 2009 13:12:52 -0500
Received: from [199.232.76.173] (port=52739 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1NBBUt-0002i1-PT
	for qemu-devel@nongnu.org; Thu, 19 Nov 2009 13:12:47 -0500
Received: from mx1.redhat.com ([209.132.183.28]:51156)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <sgrubb@redhat.com>) id 1NBBUt-0003xt-3P
	for qemu-devel@nongnu.org; Thu, 19 Nov 2009 13:12:47 -0500
From: Steve Grubb <sgrubb@redhat.com>
Subject: Re: [Qemu-devel] Fwd: qemu code review
Date: Thu, 19 Nov 2009 13:11:56 -0500
References: <4B03DD07.7090300@redhat.com> <4B0445B1.1080207@mail.berlios.de>
	<4B050B5C.4080908@redhat.com>
In-Reply-To: <4B050B5C.4080908@redhat.com>
MIME-Version: 1.0
Content-Type: Text/Plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Message-Id: <200911191311.56137.sgrubb@redhat.com>
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Kevin Wolf <kwolf@redhat.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>

On Thursday 19 November 2009 04:09:48 am Kevin Wolf wrote:
> >> ...
> >> In hw/e1000.c at line 89, vlan is declared to be 4 bytes. At line 382 is
> >> an attempt to do a memmove over it with a size of 12.
> >
> > Obviously this was intentional. Would replacing
> >         memmove(tp->vlan, tp->data, 12);
> > by
> >         memmove(tp->data - 4, tp->data, 12);
> > be better and satisfy the analysis tool?

No. Its likely point out a negative index.

> > Or even better (hopefully the compiler will combine both statements)
> >         memmove(tp->vlan, tp->data, 4);
> >         memmove(tp->data, tp->data + 4, 8);

This would make it happier. But if a comment was made that its intentionally 
overrunning the vlan array, it would cause less concern.
 
> But I think splitting it into two memmoves is better anyway. There is no
> warning in the declaration of the struct that these fields need to be
> consecutive, so someone might have the idea of reordering the fields or
> inserting a new one in between and things break...

Right. Someone might use a cache analysis tool in the future and see that it 
runs faster with reordered fields...

-Steve