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