| Message ID | 1416463034-8264-4-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 11/20/2014 01:57 PM, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > s->xmit_pos maybe assigned to a negative value (-1), > but in this branch variable s->xmit_pos as an index to > array s->buffer. Let's add a check for s->xmit_pos. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > hw/net/pcnet.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c > index d344c15..2bb6417 100644 > --- a/hw/net/pcnet.c > +++ b/hw/net/pcnet.c > @@ -1247,7 +1247,7 @@ static void pcnet_transmit(PCNetState *s) > s->xmit_pos = -1; > goto txdone; > } > - if (!GET_FIELD(tmd.status, TMDS, ENP)) { > + if (!GET_FIELD(tmd.status, TMDS, ENP) && (s->xmit_pos >= 0)) { > int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); > s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), > s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); Reviewed-by: Jason Wang <jasowang@redhat.com>
On 20/11/2014 06:57, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > s->xmit_pos maybe assigned to a negative value (-1), > but in this branch variable s->xmit_pos as an index to > array s->buffer. Let's add a check for s->xmit_pos. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > hw/net/pcnet.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c > index d344c15..2bb6417 100644 > --- a/hw/net/pcnet.c > +++ b/hw/net/pcnet.c > @@ -1247,7 +1247,7 @@ static void pcnet_transmit(PCNetState *s) > s->xmit_pos = -1; > goto txdone; > } > - if (!GET_FIELD(tmd.status, TMDS, ENP)) { > + if (!GET_FIELD(tmd.status, TMDS, ENP) && (s->xmit_pos >= 0)) { > int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); > s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), > s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); > Even better: if (s->xmit_pos < 0) { goto txdone; } bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); s->xmit_pos += bcnt; if (FIELD(tmd.status, TMDS, ENP)) { #ifdef PCNET_DEBUG printf("pcnet_transmit size=%d\n", s->xmit_pos); #endif if (CSR_LOOP(s)) { if (BCR_SWSTYLE(s) == 1) ... } since there is duplicated code in the two "if" arms. But the call is Stefan's, as he's the maintainer. Paolo
On 2014/11/20 14:36, Paolo Bonzini wrote: > > > On 20/11/2014 06:57, arei.gonglei@huawei.com wrote: >> From: Gonglei <arei.gonglei@huawei.com> >> >> s->xmit_pos maybe assigned to a negative value (-1), >> but in this branch variable s->xmit_pos as an index to >> array s->buffer. Let's add a check for s->xmit_pos. >> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> --- >> hw/net/pcnet.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c >> index d344c15..2bb6417 100644 >> --- a/hw/net/pcnet.c >> +++ b/hw/net/pcnet.c >> @@ -1247,7 +1247,7 @@ static void pcnet_transmit(PCNetState *s) >> s->xmit_pos = -1; >> goto txdone; >> } >> - if (!GET_FIELD(tmd.status, TMDS, ENP)) { >> + if (!GET_FIELD(tmd.status, TMDS, ENP) && (s->xmit_pos >= 0)) { >> int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); >> s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), >> s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); >> > > Even better: > > if (s->xmit_pos < 0) { > goto txdone; > } > bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); > s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), > s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); > s->xmit_pos += bcnt; > if (FIELD(tmd.status, TMDS, ENP)) { > #ifdef PCNET_DEBUG > printf("pcnet_transmit size=%d\n", s->xmit_pos); > #endif > if (CSR_LOOP(s)) { > if (BCR_SWSTYLE(s) == 1) > ... > } > > since there is duplicated code in the two "if" arms. Maybe not, since two branch are "if and else if" not "if and else", so this change make the below code segment's wide ... > bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); > s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), > s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); > s->xmit_pos += bcnt; ... more extensive. Best regards, -Gonglei > But the call is > Stefan's, as he's the maintainer. > > Paolo
On 20/11/2014 07:44, Gonglei wrote: > Maybe not, since two branch are "if and else if" not "if and else", > so this change make the below code segment's wide ... >> > bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); >> > s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), >> > s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); >> > s->xmit_pos += bcnt; > ... more extensive. After your patch that fixes the coverity report, they are if (a && b) else if (b) so you can change it to if (!b) goto txdone; if (a) ... else ... and then if (!b) goto txdone; <common part> if (!a) { <extra part from else> } Paolo
On 2014/11/20 15:08, Paolo Bonzini wrote: > > > On 20/11/2014 07:44, Gonglei wrote: >> Maybe not, since two branch are "if and else if" not "if and else", >> so this change make the below code segment's wide ... >>>> bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); >>>> s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), >>>> s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); >>>> s->xmit_pos += bcnt; >> ... more extensive. > > After your patch that fixes the coverity report, they are > > if (a && b) > else if (b) > > so you can change it to > > if (!b) goto txdone; > if (a) ... > else ... > > and then > > if (!b) goto txdone; > <common part> > if (!a) { > <extra part from else> > } > > Paolo I know your mean now, thanks ;) What about this below way? Maybe more clear. if (s->xmit_pos < 0) { goto txdone; } int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); s->xmit_pos += bcnt; if (!GET_FIELD(tmd.status, TMDS, ENP)) { goto txdone; } #ifdef PCNET_DEBUG printf("pcnet_transmit size=%d\n", s->xmit_pos); #endif if (CSR_LOOP(s)) { if (BCR_SWSTYLE(s) == 1) add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS); s->looptest = add_crc ? PCNET_LOOPTEST_CRC : PCNET_LOOPTEST_NOCRC; pcnet_receive(qemu_get_queue(s->nic), s->buffer, s->xmit_pos); s->looptest = 0; } else if (s->nic) qemu_send_packet(qemu_get_queue(s->nic), s->buffer, s->xmit_pos); s->csr[0] &= ~0x0008; /* clear TDMD */ s->csr[4] |= 0x0004; /* set TXSTRT */ s->xmit_pos = -1; txdone: Best regards, -Gonglei
On 20/11/2014 08:38, Gonglei wrote: > On 2014/11/20 15:08, Paolo Bonzini wrote: > >> >> >> On 20/11/2014 07:44, Gonglei wrote: >>> Maybe not, since two branch are "if and else if" not "if and else", >>> so this change make the below code segment's wide ... >>>>> bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); >>>>> s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), >>>>> s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); >>>>> s->xmit_pos += bcnt; >>> ... more extensive. >> >> After your patch that fixes the coverity report, they are >> >> if (a && b) >> else if (b) >> >> so you can change it to >> >> if (!b) goto txdone; >> if (a) ... >> else ... >> >> and then >> >> if (!b) goto txdone; >> <common part> >> if (!a) { >> <extra part from else> >> } >> >> Paolo > > I know your mean now, thanks ;) > What about this below way? Maybe more clear. As you prefer. Paolo > if (s->xmit_pos < 0) { > goto txdone; > } > int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); > s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), > s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s)); > s->xmit_pos += bcnt; > > if (!GET_FIELD(tmd.status, TMDS, ENP)) { > goto txdone; > } > > #ifdef PCNET_DEBUG > printf("pcnet_transmit size=%d\n", s->xmit_pos); > #endif > if (CSR_LOOP(s)) { > if (BCR_SWSTYLE(s) == 1) > add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS); > s->looptest = add_crc ? PCNET_LOOPTEST_CRC : PCNET_LOOPTEST_NOCRC; > pcnet_receive(qemu_get_queue(s->nic), s->buffer, s->xmit_pos); > s->looptest = 0; > } else > if (s->nic) > qemu_send_packet(qemu_get_queue(s->nic), s->buffer, > s->xmit_pos); > > s->csr[0] &= ~0x0008; /* clear TDMD */ > s->csr[4] |= 0x0004; /* set TXSTRT */ > s->xmit_pos = -1; > > txdone: > > Best regards, > -Gonglei > >
diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c index d344c15..2bb6417 100644 --- a/hw/net/pcnet.c +++ b/hw/net/pcnet.c @@ -1247,7 +1247,7 @@ static void pcnet_transmit(PCNetState *s) s->xmit_pos = -1; goto txdone; } - if (!GET_FIELD(tmd.status, TMDS, ENP)) { + if (!GET_FIELD(tmd.status, TMDS, ENP) && (s->xmit_pos >= 0)) { int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT); s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr), s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));