On Sat, 1 Nov 2025 at 08:23, Soumyajyotii Ssarkar <[email protected]> wrote: > > From: RemZapCypher <[email protected]> > > Fixing Null pointer dereference & Async/Sync IDENTICAL_BRANCHES
Commit messages should generally try to explain what the patch is doing in more detail than this. Ideally I should be able to understand from the commit message why the change makes sense and is the right way to fix the bug. > Reported-by: Stefan Hajnoczi <[email protected]> > --- > hw/scsi/ncr53c710.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/hw/scsi/ncr53c710.c b/hw/scsi/ncr53c710.c > index b3d4593b72..dd9884c8bf 100644 > --- a/hw/scsi/ncr53c710.c > +++ b/hw/scsi/ncr53c710.c > @@ -834,13 +834,11 @@ void ncr710_transfer_data(SCSIRequest *req, uint32_t > len) > } > } > > - /* Host adapter (re)connected */ > - s->current->dma_len = len; > s->command_complete = NCR710_CMD_DATA_READY; > - > if (!s->current) { > - return; > + s->current = (NCR710Request *)req->hba_private; > } > + s->current->dma_len = len; This looks odd. We already have code (the s->wait_reselect check) earlier that says "in this situation, we need to update s->current with req_hba_private and update s->current_len". Now here we're making this code unconditionally do "if s->current isn't set, set it from hba_private". This seems like it's confused about when s->current is valid and when it's not. When we call this function: * when is s->current non-NULL and valid to use? * when should we be updating s->current from req->hba_private? * when should we update s->current->dma_len? I think that either: (a) there's a particular set of conditions like s->wait_reselect that tell us when we should update s->current, and we only need to do that then, or (b) we need to do it basically unconditionally We shouldn't have both "do this on a particular condition" and then "do this always". > if (s->waiting) { > s->scntl1 |= NCR710_SCNTL1_CON; > @@ -1367,11 +1365,6 @@ again: > case PHASE_DI: > s->waiting = NCR710_WAIT_DMA; > ncr710_do_dma(s, 0); > - if (s->waiting != NCR710_WAIT_NONE) { > - /* Async - stop and wait */ > - break; > - } > - /* Sync - continue execution */ > break; > case PHASE_CO: > ncr710_do_command(s); > -- > 2.49.0 Please don't fix two unrelated issues in the same commit: these should be separate patches (you can send them as part of a 2-patch patchset). thanks -- PMM
