Int21 fixes - Update

[Jukka Heinonen]

On Sun, Aug 24, Sylvain Petreolle wrote:
>  --- Jukka Heinonen <jhei at 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 <http://www.iki.fi/jhei/>