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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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>