Re: [Qemu-devel] Fwd: qemu code review

[Steve Grubb]

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