| Message ID | 20240731143617.3391947-7-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | block: Miscellaneous minor Coverity fixes | expand |
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? Either way: Reviewed-by: Kevin Wolf <kwolf@redhat.com>
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>
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; } /* return 0 if buffer completed */
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(-)