* [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
@ 2013-06-03 15:20 Andrew Jones
2013-06-04 7:07 ` Jesse Larrew
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Andrew Jones @ 2013-06-03 15:20 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, jasowang, stefanha, mst
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 <drjones@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 & 0xffff, bytes, sz, op;
- unsigned int msh = 0xfffff, hdr = 0;
+ unsigned int msh = 0xfffff;
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
2013-06-03 15:20 [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc Andrew Jones
@ 2013-06-04 7:07 ` Jesse Larrew
2013-06-04 7:34 ` Andrew Jones
2013-06-04 8:49 ` [Qemu-devel] [PATCH v2] " Andrew Jones
2013-06-04 10:58 ` [Qemu-devel] [PATCH] " Michael S. Tsirkin
2 siblings, 1 reply; 9+ messages in thread
From: Jesse Larrew @ 2013-06-04 7:07 UTC (permalink / raw)
To: Andrew Jones; +Cc: aliguori, mst, jasowang, qemu-devel, stefanha, pbonzini
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 <drjones@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 & 0xffff, bytes, sz, op;
> - unsigned int msh = 0xfffff, hdr = 0;
> + unsigned int msh = 0xfffff;
> 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)
jlarrew@linux.vnet.ibm.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
2013-06-04 7:07 ` Jesse Larrew
@ 2013-06-04 7:34 ` Andrew Jones
2013-06-04 7:54 ` Luigi Rizzo
2013-06-04 8:33 ` Peter Maydell
0 siblings, 2 replies; 9+ messages in thread
From: Andrew Jones @ 2013-06-04 7:34 UTC (permalink / raw)
To: Jesse Larrew; +Cc: aliguori, mst, jasowang, qemu-devel, stefanha, pbonzini
----- 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 <drjones@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 & 0xffff, bytes, sz, op;
> > - unsigned int msh = 0xfffff, hdr = 0;
> > + unsigned int msh = 0xfffff;
> > 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)
> jlarrew@linux.vnet.ibm.com
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
2013-06-04 7:34 ` Andrew Jones
@ 2013-06-04 7:54 ` Luigi Rizzo
2013-06-04 8:33 ` Peter Maydell
1 sibling, 0 replies; 9+ messages in thread
From: Luigi Rizzo @ 2013-06-04 7:54 UTC (permalink / raw)
To: Andrew Jones
Cc: Anthony Liguori, Michael S. Tsirkin, jasowang, qemu-devel,
Jesse Larrew, stefanha, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]
On Tue, Jun 4, 2013 at 9:34 AM, Andrew Jones <drjones@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 <drjones@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)
> > jlarrew@linux.vnet.ibm.com
> >
> >
>
>
--
-----------------------------------------+-------------------------------
Prof. Luigi RIZZO, rizzo@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)
-----------------------------------------+-------------------------------
[-- Attachment #2: Type: text/html, Size: 5766 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
2013-06-04 7:34 ` Andrew Jones
2013-06-04 7:54 ` Luigi Rizzo
@ 2013-06-04 8:33 ` Peter Maydell
2013-06-04 8:53 ` Andrew Jones
1 sibling, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2013-06-04 8:33 UTC (permalink / raw)
To: Andrew Jones
Cc: aliguori, mst, jasowang, qemu-devel, Jesse Larrew, stefanha,
pbonzini
On 4 June 2013 08:34, Andrew Jones <drjones@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2] e1000: cleanup process_tx_desc
2013-06-03 15:20 [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc Andrew Jones
2013-06-04 7:07 ` Jesse Larrew
@ 2013-06-04 8:49 ` Andrew Jones
2013-06-04 11:02 ` Michael S. Tsirkin
2013-06-04 10:58 ` [Qemu-devel] [PATCH] " Michael S. Tsirkin
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Jones @ 2013-06-04 8:49 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, jasowang, stefanha, mst
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.
v2:
- also cleanup coding style issues in the touched lines
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
hw/net/e1000.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index e6f46f0c511e8..620f947134780 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 & 0xffff, bytes, sz, op;
- unsigned int msh = 0xfffff, hdr = 0;
+ unsigned int msh = 0xfffff;
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,16 @@ 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);
+ sz = tp->size + bytes;
+ if (sz >= 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,8 +634,9 @@ 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;
tp->vlan_needed = 0;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
2013-06-04 8:33 ` Peter Maydell
@ 2013-06-04 8:53 ` Andrew Jones
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2013-06-04 8:53 UTC (permalink / raw)
To: Peter Maydell
Cc: aliguori, mst, jasowang, qemu-devel, Jesse Larrew, stefanha,
pbonzini
----- Original Message -----
> On 4 June 2013 08:34, Andrew Jones <drjones@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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc
2013-06-03 15:20 [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc Andrew Jones
2013-06-04 7:07 ` Jesse Larrew
2013-06-04 8:49 ` [Qemu-devel] [PATCH v2] " Andrew Jones
@ 2013-06-04 10:58 ` Michael S. Tsirkin
2 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2013-06-04 10:58 UTC (permalink / raw)
To: Andrew Jones; +Cc: jasowang, pbonzini, aliguori, qemu-devel, stefanha
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 <drjones@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 & 0xffff, bytes, sz, op;
> - unsigned int msh = 0xfffff, hdr = 0;
> + unsigned int msh = 0xfffff;
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2] e1000: cleanup process_tx_desc
2013-06-04 8:49 ` [Qemu-devel] [PATCH v2] " Andrew Jones
@ 2013-06-04 11:02 ` Michael S. Tsirkin
0 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2013-06-04 11:02 UTC (permalink / raw)
To: Andrew Jones; +Cc: jasowang, pbonzini, aliguori, qemu-devel, stefanha
On Tue, Jun 04, 2013 at 10:49:48AM +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.
>
> v2:
> - also cleanup coding style issues in the touched lines
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
I've picked this one up.
Thanks,
MST
> ---
> hw/net/e1000.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index e6f46f0c511e8..620f947134780 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 & 0xffff, bytes, sz, op;
> - unsigned int msh = 0xfffff, hdr = 0;
> + unsigned int msh = 0xfffff;
> 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,16 @@ 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);
> + sz = tp->size + bytes;
> + if (sz >= 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,8 +634,9 @@ 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;
> tp->vlan_needed = 0;
> --
> 1.8.1.4
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-06-04 11:09 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-03 15:20 [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc Andrew Jones
2013-06-04 7:07 ` Jesse Larrew
2013-06-04 7:34 ` Andrew Jones
2013-06-04 7:54 ` Luigi Rizzo
2013-06-04 8:33 ` Peter Maydell
2013-06-04 8:53 ` Andrew Jones
2013-06-04 8:49 ` [Qemu-devel] [PATCH v2] " Andrew Jones
2013-06-04 11:02 ` Michael S. Tsirkin
2013-06-04 10:58 ` [Qemu-devel] [PATCH] " Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).