Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
On 06/03/2013 10:20 AM, Andrew Jones wrote:
Coverity complains about two overruns in process_tx_desc(). The
complaints are false positives, but we might as well eliminate
them. The problem is that hdr is defined as an unsigned int,
but then used to offset an array of size 65536, and another of
size 256 bytes. hdr will actually never be greater than 255
though, as it's assigned only once and to the value of
tp-hdr_len, which is an uint8_t. This patch simply gets rid of
hdr, replacing it with tp-hdr_len, which makes it consistent
with all other tp member use in the function.
Signed-off-by: Andrew Jones drjo...@redhat.com
---
hw/net/e1000.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
The logic looks sound, but checkpatch detects some style issues. See below.
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index e6f46f0c511e8..eec3e7a4524d1 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -556,7 +556,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
uint32_t txd_lower = le32_to_cpu(dp-lower.data);
uint32_t dtype = txd_lower (E1000_TXD_CMD_DEXT | E1000_TXD_DTYP_D);
unsigned int split_size = txd_lower 0x, bytes, sz, op;
-unsigned int msh = 0xf, hdr = 0;
+unsigned int msh = 0xf;
uint64_t addr;
struct e1000_context_desc *xp = (struct e1000_context_desc *)dp;
struct e1000_tx *tp = s-tx;
@@ -603,8 +603,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
addr = le64_to_cpu(dp-buffer_addr);
if (tp-tse tp-cptse) {
-hdr = tp-hdr_len;
-msh = hdr + tp-mss;
+msh = tp-hdr_len + tp-mss;
do {
bytes = split_size;
if (tp-size + bytes msh)
@@ -612,14 +611,15 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
bytes = MIN(sizeof(tp-data) - tp-size, bytes);
pci_dma_read(s-dev, addr, tp-data + tp-size, bytes);
-if ((sz = tp-size + bytes) = hdr tp-size hdr)
-memmove(tp-header, tp-data, hdr);
+if ((sz = tp-size + bytes) = tp-hdr_len
+ tp-size tp-hdr_len)
+memmove(tp-header, tp-data, tp-hdr_len);
The 'if' statement above needs some braces. Checkpatch also isn't wild about
the assignment inside of the conditional.
tp-size = sz;
addr += bytes;
if (sz == msh) {
xmit_seg(s);
-memmove(tp-data, tp-header, hdr);
-tp-size = hdr;
+memmove(tp-data, tp-header, tp-hdr_len);
+tp-size = tp-hdr_len;
}
} while (split_size -= bytes);
} else if (!tp-tse tp-cptse) {
@@ -633,7 +633,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
if (!(txd_lower E1000_TXD_CMD_EOP))
return;
-if (!(tp-tse tp-cptse tp-size hdr))
+if (!(tp-tse tp-cptse tp-size tp-hdr_len))
xmit_seg(s);
Braces here as well.
tp-tso_frames = 0;
tp-sum_needed = 0;
Although the style issues were present to begin with, we may as well take
the opportunity to fix them.
Sincerely,
Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlar...@linux.vnet.ibm.com
Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
- Original Message -
On 06/03/2013 10:20 AM, Andrew Jones wrote:
Coverity complains about two overruns in process_tx_desc(). The
complaints are false positives, but we might as well eliminate
them. The problem is that hdr is defined as an unsigned int,
but then used to offset an array of size 65536, and another of
size 256 bytes. hdr will actually never be greater than 255
though, as it's assigned only once and to the value of
tp-hdr_len, which is an uint8_t. This patch simply gets rid of
hdr, replacing it with tp-hdr_len, which makes it consistent
with all other tp member use in the function.
Signed-off-by: Andrew Jones drjo...@redhat.com
---
hw/net/e1000.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
The logic looks sound, but checkpatch detects some style issues. See below.
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index e6f46f0c511e8..eec3e7a4524d1 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -556,7 +556,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc
*dp)
uint32_t txd_lower = le32_to_cpu(dp-lower.data);
uint32_t dtype = txd_lower (E1000_TXD_CMD_DEXT | E1000_TXD_DTYP_D);
unsigned int split_size = txd_lower 0x, bytes, sz, op;
-unsigned int msh = 0xf, hdr = 0;
+unsigned int msh = 0xf;
uint64_t addr;
struct e1000_context_desc *xp = (struct e1000_context_desc *)dp;
struct e1000_tx *tp = s-tx;
@@ -603,8 +603,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc
*dp)
addr = le64_to_cpu(dp-buffer_addr);
if (tp-tse tp-cptse) {
-hdr = tp-hdr_len;
-msh = hdr + tp-mss;
+msh = tp-hdr_len + tp-mss;
do {
bytes = split_size;
if (tp-size + bytes msh)
@@ -612,14 +611,15 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc
*dp)
bytes = MIN(sizeof(tp-data) - tp-size, bytes);
pci_dma_read(s-dev, addr, tp-data + tp-size, bytes);
-if ((sz = tp-size + bytes) = hdr tp-size hdr)
-memmove(tp-header, tp-data, hdr);
+if ((sz = tp-size + bytes) = tp-hdr_len
+ tp-size tp-hdr_len)
+memmove(tp-header, tp-data, tp-hdr_len);
The 'if' statement above needs some braces. Checkpatch also isn't wild about
the assignment inside of the conditional.
tp-size = sz;
addr += bytes;
if (sz == msh) {
xmit_seg(s);
-memmove(tp-data, tp-header, hdr);
-tp-size = hdr;
+memmove(tp-data, tp-header, tp-hdr_len);
+tp-size = tp-hdr_len;
}
} while (split_size -= bytes);
} else if (!tp-tse tp-cptse) {
@@ -633,7 +633,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc
*dp)
if (!(txd_lower E1000_TXD_CMD_EOP))
return;
-if (!(tp-tse tp-cptse tp-size hdr))
+if (!(tp-tse tp-cptse tp-size tp-hdr_len))
xmit_seg(s);
Braces here as well.
tp-tso_frames = 0;
tp-sum_needed = 0;
Although the style issues were present to begin with, we may as well take
the opportunity to fix them.
Running checkpatch on the file yields
142 errors, 41 warnings
I could send a v2 that fixes the 1 error and 2 warnings found in the context
of this patch, but why? It's out of the scope of the patch (although I did
use cleanup in the summary...), and it would hardly make a dent in this
file's problems.
drew
Sincerely,
Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlar...@linux.vnet.ibm.com
Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
On Tue, Jun 4, 2013 at 9:34 AM, Andrew Jones drjo...@redhat.com wrote: - Original Message - On 06/03/2013 10:20 AM, Andrew Jones wrote: Coverity complains about two overruns in process_tx_desc(). The complaints are false positives, but we might as well eliminate them. The problem is that hdr is defined as an unsigned int, but then used to offset an array of size 65536, and another of size 256 bytes. hdr will actually never be greater than 255 though, as it's assigned only once and to the value of tp-hdr_len, which is an uint8_t. This patch simply gets rid of hdr, replacing it with tp-hdr_len, which makes it consistent with all other tp member use in the function. Signed-off-by: Andrew Jones drjo...@redhat.com --- hw/net/e1000.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) The logic looks sound, but checkpatch detects some style issues. See below. ... Although the style issues were present to begin with, we may as well take the opportunity to fix them. Running checkpatch on the file yields 142 errors, 41 warnings I could send a v2 that fixes the 1 error and 2 warnings found in the context of this patch, but why? It's out of the scope of the patch (although I did use cleanup in the summary...), and it would hardly make a dent in this file's problems. Indeed. I find it slightly annoying (and a waste of everyone's time) that patches are bounced on issues that they are not responsible for. (this happens for several other opensource projects I have been involved with). I think that a much more effective approach would be to take the patch (on the grounds that it improves the codebase), and then if the committer feels like fixing the pre-existing style issue he can do it separately, or make a comment in the commit log if he has no time (and by the same reasoning, the original submitter may be short of time). cheers luigi Many projects i have been involved with have this drew Sincerely, Jesse Larrew Software Engineer, KVM Team IBM Linux Technology Center Phone: (512) 973-2052 (T/L: 363-2052) jlar...@linux.vnet.ibm.com -- -+--- Prof. Luigi RIZZO, ri...@iet.unipi.it . Dip. di Ing. dell'Informazione http://www.iet.unipi.it/~luigi/. Universita` di Pisa TEL +39-050-2211611 . via Diotisalvi 2 Mobile +39-338-6809875 . 56122 PISA (Italy) -+---
Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
On 4 June 2013 08:34, Andrew Jones drjo...@redhat.com wrote: I could send a v2 that fixes the 1 error and 2 warnings found in the context of this patch, but why? It's out of the scope of the patch (although I did use cleanup in the summary...), and it would hardly make a dent in this file's problems. The idea is that we gradually bring the code closer into line with QEMU's standards by (a) not allowing in new code which doesn't follow the rules and (b) fixing old code where it is in areas which a patch touches. This gradually ratchets up the quality overall without being huge touch every line in a file patches (which reduce the functionality of git blame, among other things). It really isn't a very onerous requirement in my opinion. thanks -- PMM
Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
- Original Message - On 4 June 2013 08:34, Andrew Jones drjo...@redhat.com wrote: I could send a v2 that fixes the 1 error and 2 warnings found in the context of this patch, but why? It's out of the scope of the patch (although I did use cleanup in the summary...), and it would hardly make a dent in this file's problems. The idea is that we gradually bring the code closer into line with QEMU's standards by (a) not allowing in new code which doesn't follow the rules and (b) fixing old code where it is in areas which a patch touches. This gradually ratchets up the quality overall without being huge touch every line in a file patches (which reduce the functionality of git blame, among other things). It really isn't a very onerous requirement in my opinion. OK, I surrender. v2 sent. Now we just need to find some bugs in the other 180 style-violating lines in order to finish cleaning up this file :-) drew thanks -- PMM
Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
On Mon, Jun 03, 2013 at 05:20:38PM +0200, Andrew Jones wrote:
Coverity complains about two overruns in process_tx_desc(). The
complaints are false positives, but we might as well eliminate
them. The problem is that hdr is defined as an unsigned int,
but then used to offset an array of size 65536, and another of
size 256 bytes. hdr will actually never be greater than 255
though, as it's assigned only once and to the value of
tp-hdr_len, which is an uint8_t. This patch simply gets rid of
hdr, replacing it with tp-hdr_len, which makes it consistent
with all other tp member use in the function.
Signed-off-by: Andrew Jones drjo...@redhat.com
Applied, thanks.
---
hw/net/e1000.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index e6f46f0c511e8..eec3e7a4524d1 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -556,7 +556,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
uint32_t txd_lower = le32_to_cpu(dp-lower.data);
uint32_t dtype = txd_lower (E1000_TXD_CMD_DEXT | E1000_TXD_DTYP_D);
unsigned int split_size = txd_lower 0x, bytes, sz, op;
-unsigned int msh = 0xf, hdr = 0;
+unsigned int msh = 0xf;
uint64_t addr;
struct e1000_context_desc *xp = (struct e1000_context_desc *)dp;
struct e1000_tx *tp = s-tx;
@@ -603,8 +603,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
addr = le64_to_cpu(dp-buffer_addr);
if (tp-tse tp-cptse) {
-hdr = tp-hdr_len;
-msh = hdr + tp-mss;
+msh = tp-hdr_len + tp-mss;
do {
bytes = split_size;
if (tp-size + bytes msh)
@@ -612,14 +611,15 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
bytes = MIN(sizeof(tp-data) - tp-size, bytes);
pci_dma_read(s-dev, addr, tp-data + tp-size, bytes);
-if ((sz = tp-size + bytes) = hdr tp-size hdr)
-memmove(tp-header, tp-data, hdr);
+if ((sz = tp-size + bytes) = tp-hdr_len
+ tp-size tp-hdr_len)
+memmove(tp-header, tp-data, tp-hdr_len);
tp-size = sz;
addr += bytes;
if (sz == msh) {
xmit_seg(s);
-memmove(tp-data, tp-header, hdr);
-tp-size = hdr;
+memmove(tp-data, tp-header, tp-hdr_len);
+tp-size = tp-hdr_len;
}
} while (split_size -= bytes);
} else if (!tp-tse tp-cptse) {
@@ -633,7 +633,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
if (!(txd_lower E1000_TXD_CMD_EOP))
return;
-if (!(tp-tse tp-cptse tp-size hdr))
+if (!(tp-tse tp-cptse tp-size tp-hdr_len))
xmit_seg(s);
tp-tso_frames = 0;
tp-sum_needed = 0;
--
1.8.1.4
[Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
Coverity complains about two overruns in process_tx_desc(). The
complaints are false positives, but we might as well eliminate
them. The problem is that hdr is defined as an unsigned int,
but then used to offset an array of size 65536, and another of
size 256 bytes. hdr will actually never be greater than 255
though, as it's assigned only once and to the value of
tp-hdr_len, which is an uint8_t. This patch simply gets rid of
hdr, replacing it with tp-hdr_len, which makes it consistent
with all other tp member use in the function.
Signed-off-by: Andrew Jones drjo...@redhat.com
---
hw/net/e1000.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index e6f46f0c511e8..eec3e7a4524d1 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -556,7 +556,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
uint32_t txd_lower = le32_to_cpu(dp-lower.data);
uint32_t dtype = txd_lower (E1000_TXD_CMD_DEXT | E1000_TXD_DTYP_D);
unsigned int split_size = txd_lower 0x, bytes, sz, op;
-unsigned int msh = 0xf, hdr = 0;
+unsigned int msh = 0xf;
uint64_t addr;
struct e1000_context_desc *xp = (struct e1000_context_desc *)dp;
struct e1000_tx *tp = s-tx;
@@ -603,8 +603,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
addr = le64_to_cpu(dp-buffer_addr);
if (tp-tse tp-cptse) {
-hdr = tp-hdr_len;
-msh = hdr + tp-mss;
+msh = tp-hdr_len + tp-mss;
do {
bytes = split_size;
if (tp-size + bytes msh)
@@ -612,14 +611,15 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
bytes = MIN(sizeof(tp-data) - tp-size, bytes);
pci_dma_read(s-dev, addr, tp-data + tp-size, bytes);
-if ((sz = tp-size + bytes) = hdr tp-size hdr)
-memmove(tp-header, tp-data, hdr);
+if ((sz = tp-size + bytes) = tp-hdr_len
+ tp-size tp-hdr_len)
+memmove(tp-header, tp-data, tp-hdr_len);
tp-size = sz;
addr += bytes;
if (sz == msh) {
xmit_seg(s);
-memmove(tp-data, tp-header, hdr);
-tp-size = hdr;
+memmove(tp-data, tp-header, tp-hdr_len);
+tp-size = tp-hdr_len;
}
} while (split_size -= bytes);
} else if (!tp-tse tp-cptse) {
@@ -633,7 +633,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
if (!(txd_lower E1000_TXD_CMD_EOP))
return;
-if (!(tp-tse tp-cptse tp-size hdr))
+if (!(tp-tse tp-cptse tp-size tp-hdr_len))
xmit_seg(s);
tp-tso_frames = 0;
tp-sum_needed = 0;
--
1.8.1.4
