Int21 fixes - Update
jhei at iki.fi
Sun Aug 24 07:27:50 CDT 2003
I have to admit I'm a bit disappointed with this patch:
- Patch name should really be more descriptive.
- Changelog is not up to date with patch.
- This patch cannot be compiled with a C compiler. This makes it
look like this patch has not been tested at all...
- There are logic errors in the patch.
- You should really either clear CFLAG at start of each
subfunction or clear it on success. Using both approaches
makes the work of future int21 workers more difficult.
There may be cases where you need to use the other approach
but those cases should be rare.
Here is a quick review of the patch. There could
be more bugs still left in the patch:
ioctl : You really should consider all IOCTL subfunctions before trying to modify
CFLAG behaviour. Putting one top level RESET_CFLAG with FIXME comment would
be fine but fixing one branch and leaving other branches alone is not.
0x2d : not ok. First of all, illegal time is not an error. Some programs seem
to use it as installation check, see subfunction 0x2b.
Second thing is that you only reset CFLAG here and do not set it.
Third thing is that I don't think this subfunction touches CFLAG at all.
0x38 : not ok. It would be pointless to return an error here because
it only makes fewer programs work. Just add RESET_CFLAG and leave
the logic as it is. Besides you have broken bSetDOSExtendedError handling here.
0x39-0x43 : ok
0x45 : not ok. Missing brackets.
0x46-47 : ok
0x48 : ok (see 0x5c)
0x4a : This is not valid C. RESET_CFLAG cannot happen before
variable declarations. Move RESET_CFLAG to start of subfunction.
0x4b : ok
0x56 : ok
0x57 : ok
0x5b : ok
0x5c : not ok (cosmetic bug). If you decide to use
if-then-bSetDOSExtendedError-else-RESET_CFLAG you really should try to use it
everywhere. Or you could use RESET_CFLAG as first operation in each subfunction.
But try not to use mixed approaches, if it can be helped.
Jukka Heinonen <http://www.iki.fi/jhei/>
More information about the wine-devel