Re: [Qemu-devel] [PATCH 3/4] pcnet: fix Negative array index read

[Gonglei]

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