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.
0x4a : This is not valid C. RESET_CFLAG cannot happen before variable declarations. Move RESET_CFLAG to start of subfunction.
Well, this is not valid C, but is perfectly valid C99. And as most of the people now are using GCC 3.X to develop Wine, these kind of stuff happen a lot more often (ie a lot of 'breakage' of compiles happens on people still using older GCC releases).
Lionel
--- Jukka Heinonen jhei@iki.fi a écrit : > 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.
Changelog != message title, you ok? And tell why its not in sync with the patch.
- This patch cannot be compiled with a C compiler. This makes it look like this patch has not been tested at all...
I compiled it with GCC 3.X and got no problem at all.
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.
I didnt change any IOCTL function, if you see one, be more precise.
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.
Illegal time IS an error, same for the date, and is reported with AL=FF. See Ralf Brown's Interrupt List. You're right for one point : my CFLAG reset is an error here.
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.
I dont follow you here. I add RESET_CLAG in case of success. In case of failure, CFLAG is set and an extended error code is returned in AX.
0x45 : not ok. Missing brackets.
Ok, will fix it.
0x48 : ok (see 0x5c)
This is not sufficient since BX must be set too.
0x4a : This is not valid C. RESET_CFLAG cannot happen before variable declarations. Move RESET_CFLAG to start of subfunction.
If this really bothers GCC 2.x and others : the better is to put the variables declarations at the start of the subfunction.
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.
===== Sylvain Petreolle (spetreolle_at_users_dot_sourceforge_dot_net) ICQ #170597259
alias upsf='false ; while [ $? -ne 0 ] ; do cvs update -APd ; done 2>&1 |tee cvslog'
"What if tomorrow the War could be over ?" Morpheus, in "Reloaded".
___________________________________________________________ Do You Yahoo!? -- Une adresse @yahoo.fr gratuite et en français ! Yahoo! Mail : http://fr.mail.yahoo.com
On Sun, Aug 24, Sylvain Petreolle wrote:
--- Jukka Heinonen jhei@iki.fi a écrit :
- Patch name should really be more descriptive.
- Changelog is not up to date with patch.
Changelog != message title, you ok?
I'm fine, thanks. You are correct in that the message title is not that important, but I think it would be a good idea to mention that you are fixing int21 CFLAG handling.
And tell why its not in sync with the patch.
Your changelog tries to list each subfunction you have modified but the list does not include all the changes. It would be better to be either more vague and not mention separate subfunctions or to fix changelog so that you really list all the subfunctions.
- This patch cannot be compiled with a C compiler. This makes it look like this patch has not been tested at all...
I compiled it with GCC 3.X and got no problem at all.
Ok, I didn't know C99 changed C syntax about declarations. Probably I should finally go through the trouble of installing GCC 3.X...
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.
I didnt change any IOCTL function, if you see one, be more precise.
It is you who should know what your patch consists of, not me. But just to remind you, copied from patch with more context added:
# static void INT21_Ioctl_Block( CONTEXT86 *context ) # { # BYTE *dataptr; # BYTE drive = INT21_MapDrive( BL_reg(context) ); # WCHAR drivespec[4] = {'A', ':', '\', 0}; # UINT drivetype; # # drivespec[0] += drive; # drivetype = GetDriveTypeW( drivespec ); # #+ RESET_CFLAG(context); # if (drivetype == DRIVE_UNKNOWN || drivetype == DRIVE_NO_ROOT_DIR) # { # TRACE( "IOCTL - SUBFUNCTION %d - INVALID DRIVE %c:\n",
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.
Illegal time IS an error, same for the date, and is reported with AL=FF. See Ralf Brown's Interrupt List. You're right for one point : my CFLAG reset is an error here.
What I meant to say was that when the subfunction is given illegal time, you should not use ERR() function for logging the error because using illegal time is part of the logic of some programs. If you want to log that at all, use TRACE() instead. However, you have to return 0xff in AL register, just like subfunction 0x2b does. I guess I should have been more careful here in order to prevent misunderstandings.
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.
I dont follow you here. I add RESET_CLAG in case of success. In case of failure, CFLAG is set and an extended error code is returned in AX.
You set bSetDOSExtendedError flag. This flag turns CFLAG on and copies GetLastError() into AX register. However, I don't think GetLastError() is going to return the correct error code here because it has been reset to zero at the top of the same function.
Anyway, you should only add single RESET_CLAG here and you should not change the way this subfunction works because it works like that on purpose.
0x45 : not ok. Missing brackets.
Ok, will fix it.
0x48 : ok (see 0x5c)
This is not sufficient since BX must be set too.
0x4a : This is not valid C. RESET_CFLAG cannot happen before variable declarations. Move RESET_CFLAG to start of subfunction.
If this really bothers GCC 2.x and others : the better is to put the variables declarations at the start of the subfunction.
Well, I would prefer that you move RESET_CFLAG down two lines, move it at the start of the subfunction or add else branch to the following if-construct that explicitly calls RESET_CFLAG on success. I kind of like to declare variables as late as possible and having written that code I don't want that thing to change :)
And you really should decide whether you want to prefer having each branch either set or reset CFLAG or whether you prefer having RESET_CFLAG at the start of the subfunction and make failure patchs call SET_CFLAG. This subfunction mixes both approaches, which is rather error prone (this might work in simple subfunctions like 0x4a but it makes it really hard to make sure that CFLAG is handled properly in more complex subfunctions).
P.S. More respectful attitude would be appreciated. I could have already written a patch that fixes CFLAG handling during the time it has taken to comment your int21 patches and replies. However, I thought it would be better to review your patches and give you some help because submitting correct, well written patches is a skill that takes time to learn.
-
Jukka Heinonen -
Lionel Ulmer -
Sylvain Petreolle