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

[Hartmut Birr]

Copied from the archive because I will receive the mail in some hours:
>  hbirr at svn.reactos.com wrote: 
>  >- Guard the copying to the IOSB.
> >- Do the main processing on success or if previous STATUS_PENDING was 
returned.
> >   Do not use
> > some of the IRP and FO flags at this point.
> >
> >  
> Ok, I was going to complain about this, but I realize that it must be
> used since defintely some parts of ROS don't work properly with those
> flags set. 
It isn't possible to use the flags, because some functions are always
synchronous independently which flags are set. Ntoskrnl may set the
flags correctly, but a driver may possible not.

> >  - Set all
> > results before signaling the events.
> > - Signal the FO event previous the user event.
> > - Made the code a little bit shorter.
> >
> >
> >  
> I like your changes and have no complaints, except that the signaling
> semantics of the File/User events should be different in the failure
> case (my "else" path). You've completely removed that path and thus
> changed the logic. 
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.

- Hartmut

[Alex Ionescu]

Hartmut Birr wrote:

>  Copied from the archive because I will receive the mail
> in some hours:
>   
>
> >  hbirr at svn.reactos.com wrote:
> >     
> >  
>   
>
> >  >- Guard the copying to the IOSB.
> > >- Do the main processing on success or if previous STATUS_PENDING was
> > >      
> > > 
> returned.
>   
>
> > >  Do not use
> > > some of the IRP and FO flags at this point.
> > >
> > >
> > >       
> > >  
> > Ok, I was going to complain about this, but I realize that it must be
> > used since defintely some parts of ROS don't work properly with those
> > flags set.
> >     
> >  
> It isn't possible to use the flags, because some functions are always
> synchronous independently which flags are set. Ntoskrnl may set the
> flags correctly, but a driver may possible not.
>   
>  
It's not our job to make 3rd-party drivers happy. If they work on 
Windows, which uses the flags, then they must work on ReactOS. If our 
drivers are broken, then they must be fixed.

>    
>
> > >  - Set all
> > > results before signaling the events.
> > > - Signal the FO event previous the user event.
> > > - Made the code a little bit shorter.
> > >
> > >
> > >
> > >       
> > >  
> > I like your changes and have no complaints, except that the signaling
> > semantics of the File/User events should be different in the failure
> > case (my "else" path). You've completely removed that path and thus
> > changed the logic.
> >     
> >  
> 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).

Best regards,
Alex Ionescu

[Hartmut Birr]

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

[Joseph Galbraith]

>   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. 
I'm not sure how it relates to this discussion (if at all)
but I'm pretty sure that under windows, that if I have
a file handle associated with a completion port, that
the completion notification is delivered to the port
regardless of whether it pended or not.

(I depend on this behavior in my apps... it means I only
have one place where I process I/O results.)

I also believe that I can wait on the OVERLAPPED's
event to detect completion (regardless of whether
the I/O pended or not.)

Thanks,

Joseph

[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

[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

[Alex Ionescu]

Hartmut Birr wrote:

> >  
> > 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.
>   
>  
So you agree that we shouldn't do it for SYNC IRP or SYNC FO?
Good, at least we're getting somewhere then.

>    
>
> >  - 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
>   
>  
If Pending returned, then Write the IOSB, and Signal the User OR File Event.
Write IOSB. the SYNCicty of the FO is irrelevant in this path.

>  3) IRP is not SYNC, FO is not SYNC
>   
>  
If Pending returned, then Signal the FileObject Event and write 
FinalStatus. Once again, the SYNCity of the FO is Irrelevant here, so 
this also applies if the FO is sync.

>  4) IRP is SYNC, no FO
>   
>  
Nothing. Complete and utter failure. No event will be signaled, no 
status will be written.

>  - 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 
>  
It shouldn't!

>  and it doesn't return a status
> to the caller for situation three.
>  
It does, the file event will be signaled and the final status will be 
written (NOT the IOSB, but the File OBject final status, which is what 
the caller should be waiting on).

>   My code does nearly the same as the
> success part. 
>   
>  
Nearly, yes, but misses these sublle differences.

>  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.
>   
>  
Second stage is always called through APC except in the following cases:
    IoRetryIoCompletions, a special internal function running at 
APC_LEVEL when a pagefault is happening and completion is deadlocking 
for some reason.
    IopSynchronousServiceTail, an internal function which handles the 
return of system sync irps handled by ntoskrnl. The routine is     
called directly at APC level if DEFERRED_IO_COMPLETION was used by the 
driver.
    IopAbortRequest, the Rundown APC for IO Completion.

Completion port is called exactly like I coded it in my newest patch, 
and I followed the Nagar book

Once again, let me remind you that TCPIP is broken on trunk because your 
code does not return any sort of status to the caller.

Best regards,
Alex Ionescu

[Hartmut Birr]

Alex Ionescu wrote:

>  
>  Once again, let me remind you that TCPIP is broken on trunk because
>  your code does not return any sort of status to the caller.
>  
I would like to hear, what exactly breaks tcpip. I've build rev 14860,
14861, 14963, 14964 and 15025 with and without optimisation. Some of the
optimized builds (rev 14861, 14963) does crash while booting. I see no
difference for the network working. I'm using a pc with a nic from dlink
with a realtek 8139 chipset. I'm using gge's setup, except that the sub
vendor is changed to dlink. I can start a ping from ros. It reports
always a timeout error. I see the LED's from the switch flashing. The
personal firewall of the target machine reports an incoming icmp packet
with an return address of 0.0.0.0.  So I do not see something, which
breaks networking or tcpip.

- Hartmut

[Alex Ionescu]

Hartmut Birr wrote:

>  I would like to hear, what exactly breaks tcpip.
> I've build rev 14860,
> 14861, 14963, 14964 and 15025 with and without optimisation. Some of the
> optimized builds (rev 14861, 14963) does crash while booting. I see no
> difference for the network working. I'm using a pc with a nic from dlink
> with a realtek 8139 chipset. I'm using gge's setup, except that the sub
> vendor is changed to dlink. I can start a ping from ros. It reports
> always a timeout error. I see the LED's from the switch flashing. The
> personal firewall of the target machine reports an incoming icmp packet
> with an return address of 0.0.0.0.  So I do not see something, which
> breaks networking or tcpip.
>   
>  
Sorry for not being specific. Ask WaxDragon about it. TCPIP returns 
STATUS_BUFFER_TOO_SMALL sometimes and PendingReturned == 0, so your code 
doesn't write the IOSB at all, so the guy waiting for the IRP never gets 
a valid status.

You've also avoided replying to the scenarios I presented. I think it's 
unfair that I must waste time I could spend on furthering development to 
write a driver because you will not trust me.

>  - Hartmut
>
>   
>  
Best regards,
Alex Ionescu

[Gge]

Hartmut Birr wrote:

>   Alex Ionescu wrote:
>  
>  
> >  Once again, let me remind you that TCPIP is broken
> > on trunk because
> > your code does not return any sort of status to the caller.
> >  
>  
>  I would like to hear, what exactly breaks tcpip. I've build rev 14860,
>  14861, 14963, 14964 and 15025 with and without optimisation. Some of the
>  optimized builds (rev 14861, 14963) does crash while booting. I see no
>  difference for the network working. I'm using a pc with a nic from dlink
>  with a realtek 8139 chipset. I'm using gge's setup, except that the sub
>  vendor is changed to dlink. I can start a ping from ros. It reports
>  always a timeout error. I see the LED's from the switch flashing. The
>  personal firewall of the target machine reports an incoming icmp packet
>  with an return address of 0.0.0.0.  So I do not see something, which
>  breaks networking or tcpip.
>  
>  - Hartmut 
Harmut,

Ping works ok.
Ibrowser and ftp have the same bugcheck .
Bug 619 created for ibrowser.

Note : I have a static ip

Regards
Gerard