Re: 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