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

[Hartmut Birr]

Alex Ionescu wrote:

> 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.
>
> 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.
>
I agree with this.

> - 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
>
I do not completley agree with this. There exist four situations on error:

1) IRP is SYNC, FO is SYNC
2) IRP is SYNC, FO is not SYNC
3) IRP is not SYNC, FO is not SYNC
4) IRP is SYNC, no FO

- IRP FAILURE or PendingReturned with a SYNC Operation:
     - If PendingReturned (always with and without a FO):
          - Write IOSB.
          - If There is a User Event, Signal it
     - If PendingReturned with a FO:
          - If the FO is SYNC or if there is no User Event, Signal the
FO Event
          - If the IRP is SYNC, Write FinelStatus in the FO Event
- Cleanup & free code

Your code doesn't handle situation four and it doesn't return a status
to the caller for situation three. My code does nearly the same as the
success part. 

I'm not sure what we must do for a FO with a completion port. If I
understand Nagar's 'File System Internals' correctly, the second stage
is execute directly and not as an APC. The completion port is called always.

> 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.
>
- Hartmut