Re: Was: [ros-dev] Re: [ros-svn] [hbirr] 14988: - Guard the copying to the IOSB.

[Alex Ionescu]

Hartmut Birr wrote:

>  Alex Ionescu wrote:
>
>   
>
> >  Hartmut Birr wrote:
> >
> >     
> >
> > >  If the driver has returned STATUS_PENDING and
> > > completes the irp later
> > > with an error, both events must be signaled, because two different
> > > threads may waiting on this events. The IOSB must be also set. The apc
> > > must be also deliver (I've tested this with the apc sample and some
> > > modifications on windows). There is no difference between completing the
> > > irp with and without an error if the driver has returned STATUS_PENDING.
> > >
> > >       
> > >  
> > Your code now says "    if (NT_SUCCESS(Irp->IoStatus.Status) ||
> > Irp->PendingReturned)". Not only is this incorrect because the flag
> > must be checked, but my code had a path which handled this being
> > FALSE. Currently ros does not have this path anymore after your
> > changes. So if IoStatus.Status is a failure and PendingReturned is not
> > set, nothing will be done. In my version of the code (which is what NT
> > does), the events were still signaled in a specific case (see old code).
> >
> >     
> >  
> If a driver returns immediately with an error status, it isn't necessary
> to signal any event. The caller does only wait on the event, if the
> driver returns STATUS_PENDING (Some user mode functions may only check
> for an error). If the driver returns STATUS_PENDING, the driver has
> called  IoMarkIrpPending , which sets Irp->PendingReturned. The error
> path without STATUS_PENDING must not do anything.
>
> - Hartmut
>   
>  
This is not true, and I'm currently trying to find some time to write a 
driver to prove you otherwise, but thankfully I've noticed that this 
breaks tcpip! Tcpip returns with STATUS_BUFFER_OVERFLOW. The caller is 
never notified because you don't set the IO STATUS BLOCK unless there 
was success.

There are also other differences that I will try to prove to you with a 
driver:
1) On ReactOS if you fail an IRP, have PendingReturned to TRUE but are 
either IRP_SYNCHRONOUS_API or FO_SYNCHRONOUS_IO, the UserEvent will get 
signaled. On Windows, it won't, unless you have a FileObject and a 
SYNC_API IRP.
2) On ReactOS, if you fail an IRP, have PendingReturned to TRUE but are 
FO_SYNCHRONOUS_IO and IRP_SYNCHRONOUS_API,  and specificed a user event, 
the file event will get signaled. On Windows, it won't.

And others.

These are all the results of not handling the code paths separately like 
I did, with the following correct logic:

- IRP SUCCESS or PendingReturned with an ASYNC Operation:
  - Write IOSB
  - Notifiy File Object Event if FO_SYNC_IO.
  - Notify User Event
  - Do APC, Completion
  - Clean up & free code

This is currently done correctly, but is also being done for SYNC. This 
is wrong.

- IRP FAILURE or PendingReturned with a SYNC Operation:
      - If PendingReturned with a FileObject, and the IRP is sync:
           - Write IOSB.
           - If There is a User Event, Signal it
           - If there isn't, signal the FileObject Event instead (note, 
do NOT signal both)
      - If PendingReturned with a FileObject, and the IRP is async:
           - Write FinalStatus in the File Object
           - Signal the FileObject Event
- Cleanup & free code

This is not done correctly at all because PendingReturned with a SYNC 
Operation is not treated like I've written.

If I sound a bit pissed right now, it's because I am. I realize I 
might've broken some things with these changes by accident, and I know 
you're trying to help, but right now we are no better with networking 
dead and the differences I mentionned. I will try to find time to write 
a driver to prove to you point by poitn what I am saying, since it 
doesn't look like we can find comon grond.

Best regards,
Alex Ionescu