On 5/18/21 5:01 AM, P J P wrote:
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.
(I know this is extremely backwards from how good code ought to be
written where we centralize protecting sane values from becoming object
state.)
| > 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.)
|
(Which, to be clear, is 100% OK by me for this patch.)
| > - 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.
Understand. Maybe I can help. The fuzzer reproducer is a great first
step, but just needs to be "back-translated" into the logical operations
it is performing so that the test code is readable.
I started doodling a tracer patch similar to the IDE one I checked in
some ages ago to give symbolic names to the registers on read/write,
which makes "reading" Alexander's fuzzing reproducers a bit easier.
I'll go work on that for a little while.
* I'll check and revise the patch with above details asap.
OK; on your own schedule. I will try to leap on the patches as soon as I
get them before the FDC code falls out of my head again.
If at all possible, I wouldn't mind seeing a series bundled with the
other FDC fixes outstanding aggregated together. It will be easier (for
me) to make sure I have everything up to date and together. (If it isn't
too much hassle for you.)
AFAIK there's one for reads and one for writes that are very similar --
they protect against null BLK reads when we do not have a floppy drive
present.
Thank you,
--js
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D