Hello John,
+-- On Mon, 17 May 2021, John Snow wrote --+
| > /* Selected drive */
| > - fdctrl->cur_drv = value & FD_DOR_SELMASK;
| > + if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
| > + fdctrl->cur_drv = value & FD_DOR_SELMASK;
| > + }
|
| I don't think this is correct. If you look at get_cur_drv(), it uses the
| TDR_BOOTSEL bit to change the logical mappings of "drive 0" or "drive 1" to be
| reversed. You don't check that bit here, so you might be checking the wrong
| drive.
|
| Plus, the TDR bit can change later, so I think you shouldn't actually protect
| the register write like this. Just delete this bit of code. We ought to
| protect the drives when we go to use them instead of preventing the registers
| from getting "the wrong values".
* I see.
| > cur_drv = get_cur_drv(fdctrl);
| > + if (!cur_drv->blk) {
| > + FLOPPY_DPRINTF("No drive connected\n");
| > + return 0;
| > + }
|
| This seems fine ... or at least not worse than the other error handling we
| already have here. (Which seems to be ... basically, none. We just ignore the
| write and do nothing, which seems wrong. I guess it's better than a crash...
| but I don't have the time to do a proper audit of what this is SUPPOSED to do
| in this case.)
|
| > - if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
| > + if (cur_drv->blk == NULL
| > + || blk_pwrite(cur_drv->blk, fd_offset(cur_drv),
| > fdctrl->fifo,
|
| Seems fine, but if we had a drive for the earlier check, will we really be in
| a situation where we don't have one now?
|
| Ignore the bit I sent earlier about the qtest reproducer not correlating to
| this patch -- it does, I was experiencing an unrelated crash.
* Okay.
| On 5/17/21 7:12 AM, P J P wrote:
| > Do we need a revised patch[-series] including a qtest? OR it can be done at
| > merge time?
|
| If you have the time to write a qtest reproducer, you can send it separately
| and I'll pick it up if everything looks correct.
* Yes, that seems better, I'll try to create a qtest, but it may take time.
* I'll check and revise the patch with above details asap.
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
