Patchwork e1000: Fix warning from code review

login
register
mail settings
Submitter Stefan Weil
Date Nov. 19, 2009, 6:44 p.m.
Message ID <1258656295-5688-1-git-send-email-weil@mail.berlios.de>
Download mbox | patch
Permalink /patch/38865/
State New
Headers show

Comments

Stefan Weil - Nov. 19, 2009, 6:44 p.m.
A code review run by Steve Grubb complained about code in e1000.c:

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.

This was fixed by splitting the memmove in two calls and
adding a comment to the declaration of vlan and data.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/e1000.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Ian Molton - Nov. 19, 2009, 8:16 p.m.
Stefan Weil wrote:
> A code review run by Steve Grubb complained about code in e1000.c:
> 
> 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.

> +        /* Fields vlan and data must not be reordered or separated. */
>          unsigned char vlan[4];
>          unsigned char data[0x10000];

Wouldnt it be better to stuff both into a struct or something? I guess
from the '12' that the data size can vary, but thats less important if
they are packed in a way that the compiler (and coders!) know not to
seperate them.

-Ian

Patch

diff --git a/hw/e1000.c b/hw/e1000.c
index 00f6a57..6f9adb5 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -89,6 +89,7 @@  typedef struct E1000State_st {
     struct e1000_tx {
         unsigned char header[256];
         unsigned char vlan_header[4];
+        /* Fields vlan and data must not be reordered or separated. */
         unsigned char vlan[4];
         unsigned char data[0x10000];
         uint16_t size;
@@ -383,7 +384,8 @@  xmit_seg(E1000State *s)
     if (tp->sum_needed & E1000_TXD_POPTS_IXSM)
         putsum(tp->data, tp->size, tp->ipcso, tp->ipcss, tp->ipcse);
     if (tp->vlan_needed) {
-        memmove(tp->vlan, tp->data, 12);
+        memmove(tp->vlan, tp->data, 4);
+        memmove(tp->data, tp->data + 4, 8);
         memcpy(tp->data + 8, tp->vlan_header, 4);
         qemu_send_packet(s->vc, tp->vlan, tp->size + 4);
     } else