From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35152) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UjllF-0003SG-HL for qemu-devel@nongnu.org; Tue, 04 Jun 2013 03:34:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UjllA-0002gu-AH for qemu-devel@nongnu.org; Tue, 04 Jun 2013 03:34:29 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:43510) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UjllA-0002gk-1N for qemu-devel@nongnu.org; Tue, 04 Jun 2013 03:34:24 -0400 Date: Tue, 4 Jun 2013 03:34:21 -0400 (EDT) From: Andrew Jones Message-ID: <392488894.3387176.1370331261300.JavaMail.root@redhat.com> In-Reply-To: <51AD9226.4010900@linux.vnet.ibm.com> References: <1370272838-15373-1-git-send-email-drjones@redhat.com> <51AD9226.4010900@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jesse Larrew Cc: aliguori@us.ibm.com, mst@redhat.com, jasowang@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, pbonzini@redhat.com ----- 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 > > --- > > 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 > >