Re: [PATCH 6/7] hw/ide/pci.c: Remove dead code from bmdma_prepare_buf()

[Philippe Mathieu-Daudé]

On 31/7/24 17:13, Kevin Wolf wrote:
> Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> > Coverity notes that the code at the end of the loop in
> > bmdma_prepare_buf() is unreachable.  This is because in commit
> > 9fbf0fa81fca8f527 ("ide: remove hardcoded 2GiB transactional limit")
> > we removed the only codepath in the loop which could "break" out of
> > it, but didn't notice that this meant we should also remove the code
> > at the end of the loop.
> >
> > Remove the dead code.
> >
> > Resolves: Coverity CID 1547772
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   hw/ide/pci.c | 4 ----
> >   1 file changed, 4 deletions(-)
> >
> > diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> > index 4675d079a17..f2cb500a94f 100644
> > --- a/hw/ide/pci.c
> > +++ b/hw/ide/pci.c
> > @@ -266,10 +266,6 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma, 
> > int32_t limit)
> >               s->io_buffer_size += l;
> >           }
> >       }
> > -
> > -    qemu_sglist_destroy(&s->sg);
> > -    s->io_buffer_size = 0;
> > -    return -1;
> >   }
>
> Should we put a g_assert_not_reached() here instead to make it easier
> for the reader to understand how this function works?

Or break and keep returning at EOF:

-- >8 --
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4675d079a1..8386c4fe42 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -237,7 +237,7 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma, 
int32_t limit)
             /* end of table (with a fail safe of one page) */
             if (bm->cur_prd_last ||
                 (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
-                return s->sg.size;
+                break;
             }
             pci_dma_read(pci_dev, bm->cur_addr, &prd, 8);
             bm->cur_addr += 8;
@@ -267,9 +267,7 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma, 
int32_t limit)
         }
     }

-    qemu_sglist_destroy(&s->sg);
-    s->io_buffer_size = 0;
-    return -1;
+    return s->sg.size;
 }

---

> Either way:
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>