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
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
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
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
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
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:
- 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
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:
- 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:
- IRP is SYNC, FO is SYNC
- 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.
- 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.
- 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
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
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
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
-
Alex Ionescu -
Gge -
Hartmut Birr -
Joseph Galbraith