[ros-diffs] [ion] 22704: - Fix I/O Completion (IopCompleteRequest/IofCompleteRequest) for the first time after the 40-thread long flame war last year: - Don't read pointers from the file object or IRP before they are actually used, because in some parts of the code, these pointers could change before we actually use them. - Get rid of the #if 1/#if 0 nonsense hbirr had added. - Properly check for success/warning/failure cases (thanks to Filip for checking this out with me last year) - Handle scenarios when the IRP is marked IRP_CREATE_OPERATION - Bugcheck if IofCompleteRequest is called twice on the same IRP - Copy the master IRP's thread to the associated IRP - Free the Auxiliary Buffer if there is one. - Some formatting fixes, and majorly recomment the code to make it a lot clearer and more verbose on some of the more intricate details. - Remove some hacks which I don't think are needed anymore. If you notice regressions due to this patch let me know ASAP.

ion at svn.reactos.org ion at svn.reactos.org
Fri Jun 30 05:15:56 CEST 2006


Author: ion
Date: Fri Jun 30 07:15:56 2006
New Revision: 22704

URL: http://svn.reactos.org/svn/reactos?rev=22704&view=rev
Log:
- Fix I/O Completion (IopCompleteRequest/IofCompleteRequest) for the first time after the 40-thread long flame war last year:
  - Don't read pointers from the file object or IRP before they are actually used, because in some parts of the code, these pointers could change before we actually use them.
  - Get rid of the #if 1/#if 0 nonsense hbirr had added.
  - Properly check for success/warning/failure cases (thanks to Filip for checking this out with me last year)
  - Handle scenarios when the IRP is marked IRP_CREATE_OPERATION
  - Bugcheck if IofCompleteRequest is called twice on the same IRP
  - Copy the master IRP's thread to the associated IRP
  - Free the Auxiliary Buffer if there is one.
  - Some formatting fixes, and majorly recomment the code to make it a lot clearer and more verbose on some of the more intricate details.
  - Remove some hacks which I don't think are needed anymore. If you notice regressions due to this patch let me know ASAP.

Modified:
    trunk/reactos/ntoskrnl/include/internal/io.h
    trunk/reactos/ntoskrnl/io/irp.c

Modified: trunk/reactos/ntoskrnl/include/internal/io.h
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/io.h?rev=22704&r1=22703&r2=22704&view=diff
==============================================================================
--- trunk/reactos/ntoskrnl/include/internal/io.h (original)
+++ trunk/reactos/ntoskrnl/include/internal/io.h Fri Jun 30 07:15:56 2006
@@ -62,6 +62,15 @@
                                Count])                  \
         :                                               \
         FIELD_OFFSET(CM_RESOURCE_LIST, List)
+
+//
+// Determines if the IRP is Synchronous
+//
+#define IsIrpSynchronous(Irp, FileObject)               \
+    ((Irp->Flags & IRP_SYNCHRONOUS_API)  ||             \
+     (!(FileObject) ?                                   \
+        FALSE :                                         \
+        FileObject->Flags & FO_SYNCHRONOUS_IO))         \
 
 /*
  * VOID

Modified: trunk/reactos/ntoskrnl/io/irp.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/irp.c?rev=22704&r1=22703&r2=22704&view=diff
==============================================================================
--- trunk/reactos/ntoskrnl/io/irp.c (original)
+++ trunk/reactos/ntoskrnl/io/irp.c Fri Jun 30 07:15:56 2006
@@ -13,6 +13,8 @@
 #include <ntoskrnl.h>
 #define NDEBUG
 #include <internal/debug.h>
+
+ULONG IopTraceLevel = IO_IRP_DEBUG;
 
 /* PRIVATE FUNCTIONS  ********************************************************/
 
@@ -162,21 +164,17 @@
     PFILE_OBJECT FileObject;
     PIRP Irp;
     PMDL Mdl;
-    PKEVENT UserEvent;
-    BOOLEAN SyncIrp;
-
-    if (Apc) DPRINT("IoSecondStageCompletition with APC: 0x%p\n", Apc);
+    PVOID Port = NULL, Key = NULL;
+    BOOLEAN SignaledCreateRequest = FALSE;
 
     /* Get data from the APC */
-    FileObject = (PFILE_OBJECT)(*SystemArgument1);
+    FileObject = (PFILE_OBJECT)*SystemArgument1;
     Irp = CONTAINING_RECORD(Apc, IRP, Tail.Apc);
-    DPRINT("IoSecondStageCompletition, 0x%p\n", Irp);
-
-    /* Save the User Event */
-    UserEvent = Irp->UserEvent;
-
-    /* Remember if the IRP is Sync or not */
-    SyncIrp = Irp->Flags & IRP_SYNCHRONOUS_API ? TRUE : FALSE;
+    IOTRACE(IO_IRP_DEBUG,
+            "%s - Completing IRP %p for %p\n",
+            __FUNCTION__,
+            Irp,
+            FileObject);
 
     /* Handle Buffered case first */
     if (Irp->Flags & IRP_BUFFERED_IO)
@@ -195,6 +193,7 @@
         /* Also check if we should de-allocate it */
         if (Irp->Flags & IRP_DEALLOCATE_BUFFER)
         {
+            /* Deallocate it */
             ExFreePoolWithTag(Irp->AssociatedIrp.SystemBuffer, TAG_SYS_BUF);
         }
     }
@@ -210,17 +209,25 @@
         IoFreeMdl(Mdl);
     }
 
-    /* Remove the IRP from the list of Thread Pending IRPs */
-    RemoveEntryList(&Irp->ThreadListEntry);
-    InitializeListHead(&Irp->ThreadListEntry);
-
-#if 1
-    /* Check for Success but allow failure for Async IRPs */
-    if (!(NT_ERROR(Irp->IoStatus.Status)) ||
-        ((NT_ERROR(Irp->IoStatus.Status)) &&
-        (Irp->PendingReturned) && !(SyncIrp) &&
-        ((FileObject == NULL) || (FileObject->Flags & FO_SYNCHRONOUS_IO))))
-    {
+    /*
+     * Check if either the request was completed without any errors
+     * (but warnings are OK!), or if it was completed with an error, but
+     * did return from a pending I/O Operation and is not synchronous.
+     */
+    if ((!NT_ERROR(Irp->IoStatus.Status)) ||
+        (NT_ERROR(Irp->IoStatus.Status) &&
+        (Irp->PendingReturned) &&
+        !(IsIrpSynchronous(Irp, FileObject))))
+    {
+        /* Get any information we need from the FO before we kill it */
+        if ((FileObject) && (FileObject->CompletionContext))
+        {
+            /* Save Completion Data */
+            Port = FileObject->CompletionContext->Port;
+            Key = FileObject->CompletionContext->Key;
+        }
+
+        /* Use SEH to make sure we don't write somewhere invalid */
         _SEH_TRY
         {
             /*  Save the IOSB Information */
@@ -232,47 +239,54 @@
         }
         _SEH_END;
 
-        /* Check if there's an event */
-        if (UserEvent)
-        {
-            /* Check if we should signal the File Object Event as well */
+        /* Check if we have an event or a file object */
+        if (Irp->UserEvent)
+        {
+            /* At the very least, this is a PKEVENT, so signal it always */
+            KeSetEvent(Irp->UserEvent, 0, FALSE);
+
+            /* Check if we also have a File Object */
             if (FileObject)
             {
                 /*
-                 * If the File Object is SYNC, then we need to
-                 * signal its event too
+                 * Now, if this is a Synch I/O File Object, then this event is
+                 * NOT an actual Executive Event, so we won't dereference it,
+                 * and instead, we will signal the File Object
                  */
                 if (FileObject->Flags & FO_SYNCHRONOUS_IO)
                 {
-                    /* Set the Status */
+                    /* Signal the file object and set the status */
+                    KeSetEvent(&FileObject->Event, 0, FALSE);
                     FileObject->FinalStatus = Irp->IoStatus.Status;
-
-                    /* Signal Event */
-                    KeSetEvent(&FileObject->Event, 0, FALSE);
                 }
-            }
-
-            /* Signal the Event */
-            KeSetEvent(UserEvent, 0, FALSE);
-
-            /* Dereference the Event if this is an ASYNC IRP */
-            if (FileObject && !SyncIrp && UserEvent != &FileObject->Event)
-            {
-                ObDereferenceObject(UserEvent);
+
+                /*
+                 * This could also be a create operation, in which case we want
+                 * to make sure there's no APC fired.
+                 */
+                if (Irp->Flags & IRP_CREATE_OPERATION)
+                {
+                    /* Clear the APC Routine and remmeber this */
+                    Irp->Overlay.AsynchronousParameters.UserApcRoutine = NULL;
+                    SignaledCreateRequest = TRUE;
+                }
             }
         }
         else if (FileObject)
         {
-            /* Set the Status */
+            /* Signal the file object and set the status */
+            KeSetEvent(&FileObject->Event, 0, FALSE);
             FileObject->FinalStatus = Irp->IoStatus.Status;
-
-            /* Signal the File Object Instead */
-            KeSetEvent(&FileObject->Event, 0, FALSE);
-        }
-
-        /* Now call the User APC if one was requested */
+        }
+
+        /* Now that we've signaled the events, de-associate the IRP */
+        RemoveEntryList(&Irp->ThreadListEntry);
+        InitializeListHead(&Irp->ThreadListEntry);
+
+        /* Now check if a User APC Routine was requested */
         if (Irp->Overlay.AsynchronousParameters.UserApcRoutine)
         {
+            /* Initialize it */
             KeInitializeApc(&Irp->Tail.Apc,
                             KeGetCurrentThread(),
                             CurrentApcEnvironment,
@@ -284,179 +298,93 @@
                             Irp->
                             Overlay.AsynchronousParameters.UserApcContext);
 
-            KeInsertQueueApc(&Irp->Tail.Apc,
-                             Irp->UserIosb,
-                             NULL,
-                             2);
-            Irp = NULL;
-        }
-        else if (FileObject && FileObject->CompletionContext)
-        {
-            /* Call the IO Completion Port if we have one, instead */
-            Irp->Tail.CompletionKey = FileObject->CompletionContext->Key;
+            /* Queue it */
+            KeInsertQueueApc(&Irp->Tail.Apc, Irp->UserIosb, NULL, 2);
+        }
+        else if ((Port) &&
+                 (Irp->Overlay.AsynchronousParameters.UserApcContext))
+        {
+            /* We have an I/O Completion setup... create the special Overlay */
+            Irp->Tail.CompletionKey = Key;
             Irp->Tail.Overlay.PacketType = IrpCompletionPacket;
-            KeInsertQueue(FileObject->CompletionContext->Port,
-                          &Irp->Tail.Overlay.ListEntry);
-            Irp = NULL;
+            KeInsertQueue(Port, &Irp->Tail.Overlay.ListEntry);
+        }
+        else
+        {
+            /* Free the IRP since we don't need it anymore */
+            IoFreeIrp(Irp);
+        }
+
+        /* Check if we have a file object that wasn't part of a create */
+        if ((FileObject) && !(SignaledCreateRequest))
+        {
+            /* Dereference it, since it's not needed anymore either */
+            ObDereferenceObject(FileObject);
         }
     }
     else
     {
         /*
-         * Signal the Events only if PendingReturned and we have a File Object
+         * Either we didn't return from the request, or we did return but this
+         * request was synchronous.
          */
-        if (FileObject && Irp->PendingReturned)
-        {
-            /* Check for SYNC IRP */
-            if (SyncIrp)
-            {
-                /* Set the status in this case only */
-                _SEH_TRY
+        if ((Irp->PendingReturned) && (FileObject))
+        {
+            /* So we did return with a synch operation, was it the IRP? */
+            if (Irp->Flags & IRP_SYNCHRONOUS_API)
+            {
+                /* Yes, this IRP was synchronous, so retrn the I/O Status */
+                *Irp->UserIosb = Irp->IoStatus;
+
+                /* Now check if the user gave an event */
+                if (Irp->UserEvent)
                 {
-                    *Irp->UserIosb = Irp->IoStatus;
-                }
-                _SEH_HANDLE
-                {
-                    /* Ignore any error */
-                }
-                _SEH_END;
-
-                /* Signal our event if we have one */
-                if (UserEvent)
-                {
-                    KeSetEvent(UserEvent, 0, FALSE);
+                    /* Signal it */
+                    KeSetEvent(Irp->UserEvent, 0, FALSE);
                 }
                 else
                 {
-                    /* Signal the File's Event instead */
+                    /* No event was given, so signal the FO instead */
                     KeSetEvent(&FileObject->Event, 0, FALSE);
                 }
             }
             else
             {
-#if 1
-                /* FIXME: This is necessary to fix bug #609 */
-                _SEH_TRY
-                {
-                    *Irp->UserIosb = Irp->IoStatus;
-                }
-                _SEH_HANDLE
-                {
-                    /* Ignore any error */
-                }
-                _SEH_END;
-#endif
-                /* We'll report the Status to the File Object, not the IRP */
+                /*
+                 * It's not the IRP that was synchronous, it was the FO
+                 * that was opened this way. Signal its event.
+                 */
                 FileObject->FinalStatus = Irp->IoStatus.Status;
-
-                /* Signal the File Object ONLY if this was Async */
                 KeSetEvent(&FileObject->Event, 0, FALSE);
             }
         }
 
-        /* Dereference the Event if it's an ASYNC IRP on a File Object */
-        if (UserEvent && !SyncIrp && FileObject)
-        {
-            if (UserEvent != &FileObject->Event)
-            {
-                ObDereferenceObject(UserEvent);
-            }
-        }
-    }
-
-    /* Dereference the File Object */
-    if (FileObject) ObDereferenceObject(FileObject);
-
-    /* Free the IRP */
-    if (Irp) IoFreeIrp(Irp);
-
-#else
-
-    if (NT_SUCCESS(Irp->IoStatus.Status) || Irp->PendingReturned)
-    {
-        _SEH_TRY
-        {
-            /*  Save the IOSB Information */
-            *Irp->UserIosb = Irp->IoStatus;
-        }
-        _SEH_HANDLE
-        {
-            /* Ignore any error */
-        }
-        _SEH_END;
-
-        if (FileObject)
-        {
-            if (FileObject->Flags & FO_SYNCHRONOUS_IO)
-            {
-                /* Set the Status */
-                FileObject->FinalStatus = Irp->IoStatus.Status;
-
-                /* FIXME: Remove this check when I/O code is fixed */
-                if (UserEvent != &FileObject->Event)
-                {
-                    /* Signal Event */
-                    KeSetEvent(&FileObject->Event, 0, FALSE);
-                }
-            }
-        }
-
-        /* Signal the user event, if one exist */
-        if (UserEvent)
-        {
-            KeSetEvent(UserEvent, 0, FALSE);
-        }
-
-        /* Now call the User APC if one was requested */
-        if (Irp->Overlay.AsynchronousParameters.UserApcRoutine)
-        {
-            KeInitializeApc(&Irp->Tail.Apc,
-                            KeGetCurrentThread(),
-                            CurrentApcEnvironment,
-                            IopFreeIrpKernelApc,
-                            IopAbortIrpKernelApc,
-                            (PKNORMAL_ROUTINE)Irp->
-                            Overlay.AsynchronousParameters.UserApcRoutine,
-                            Irp->RequestorMode,
-                            Irp->
-                            Overlay.AsynchronousParameters.UserApcContext);
-
-            KeInsertQueueApc(&Irp->Tail.Apc,
-                             Irp->UserIosb,
-                             NULL,
-                             2);
-            Irp = NULL;
-        }
-        else if (FileObject && FileObject->CompletionContext)
-        {
-            /* Call the IO Completion Port if we have one, instead */
-            IoSetIoCompletion(FileObject->CompletionContext->Port,
-                              FileObject->CompletionContext->Key,
-                              Irp->
-                              Overlay.AsynchronousParameters.UserApcContext,
-                              Irp->IoStatus.Status,
-                              Irp->IoStatus.Information,
-                              FALSE);
-            Irp = NULL;
-        }
-    }
-
-    /* Free the Irp if it hasn't already */
-    if (Irp) IoFreeIrp(Irp);
-
-    if (FileObject)
-    {
-        /* Dereference the user event, if it is an event object */
-        /* FIXME: Remove last check when I/O code is fixed */
-        if (UserEvent && !SyncIrp && UserEvent != &FileObject->Event)
-        {
-            ObDereferenceObject(UserEvent);
-        }
-
-        /* Dereference the File Object */
-        ObDereferenceObject(FileObject);
-    }
-#endif
+        /* Now that we got here, we do this for incomplete I/Os as well */
+        if ((FileObject) && !(Irp->Flags & IRP_CREATE_OPERATION))
+        {
+            /* Dereference the File Object unless this was a create */
+            ObDereferenceObject(FileObject);
+        }
+
+        /*
+         * Check if this was an Executive Event (remember that we know this
+         * by checking if the IRP is synchronous)
+         */
+        if ((Irp->UserEvent) &&
+            (FileObject) &&
+            !(Irp->Flags & IRP_SYNCHRONOUS_API))
+        {
+            /* This isn't a PKEVENT, so dereference it */
+            ObDereferenceObject(Irp->UserEvent);
+        }
+
+        /* Now that we've signaled the events, de-associate the IRP */
+        RemoveEntryList(&Irp->ThreadListEntry);
+        InitializeListHead(&Irp->ThreadListEntry);
+
+        /* Free the IRP as well */
+        IoFreeIrp(Irp);
+    }
 }
 
 /* FUNCTIONS *****************************************************************/
@@ -1030,46 +958,43 @@
  */
 VOID
 FASTCALL
-IofCompleteRequest(PIRP Irp,
-                   CCHAR PriorityBoost)
+IofCompleteRequest(IN PIRP Irp,
+                   IN CCHAR PriorityBoost)
 {
     PIO_STACK_LOCATION StackPtr;
     PDEVICE_OBJECT DeviceObject;
-    PFILE_OBJECT FileObject = Irp->Tail.Overlay.OriginalFileObject;
-    PETHREAD Thread = Irp->Tail.Overlay.Thread;
+    PFILE_OBJECT FileObject;
+    PETHREAD Thread;
     NTSTATUS Status;
     PMDL Mdl;
-
+    ULONG MasterIrpCount;
+    PIRP MasterIrp;
+    IOTRACE(IO_IRP_DEBUG,
+            "%s - Completing IRP %p\n",
+            __FUNCTION__,
+            Irp);
+
+    /* Make sure this IRP isn't getting completed more then once */
+    if ((Irp->CurrentLocation) > (Irp->StackCount + 1))
+    {
+        /* Bugcheck */
+        KeBugCheckEx(MULTIPLE_IRP_COMPLETE_REQUESTS, (ULONG_PTR)Irp, 0, 0, 0);
+    }
+
+    /* Some sanity checks */
     ASSERT(KeGetCurrentIrql() <= DISPATCH_LEVEL);
     ASSERT(!Irp->CancelRoutine);
     ASSERT(Irp->IoStatus.Status != STATUS_PENDING);
 
-    /* Get the Current Stack */
+    /* Get the Current Stack and skip it */
     StackPtr = IoGetCurrentIrpStackLocation(Irp);
     IoSkipCurrentIrpStackLocation(Irp);
 
     /* Loop the Stacks and complete the IRPs */
-    for (;Irp->CurrentLocation <= (Irp->StackCount + 1); StackPtr++)
+    do
     {
         /* Set Pending Returned */
         Irp->PendingReturned = StackPtr->Control & SL_PENDING_RETURNED;
-
-        /*
-         * Completion routines expect the current irp stack location to be the same as when
-         * IoSetCompletionRoutine was called to set them. A side effect is that completion
-         * routines set by highest level drivers without their own stack location will receive
-         * an invalid current stack location (at least it should be considered as invalid).
-         * Since the DeviceObject argument passed is taken from the current stack, this value
-         * is also invalid (NULL).
-         */
-        if (Irp->CurrentLocation == (Irp->StackCount + 1))
-        {
-            DeviceObject = NULL;
-        }
-        else
-        {
-            DeviceObject = IoGetCurrentIrpStackLocation(Irp)->DeviceObject;
-        }
 
         /* Check if there is a Completion Routine to Call */
         if ((NT_SUCCESS(Irp->IoStatus.Status) &&
@@ -1078,44 +1003,65 @@
              (StackPtr->Control & SL_INVOKE_ON_ERROR)) ||
             (Irp->Cancel && (StackPtr->Control & SL_INVOKE_ON_CANCEL)))
         {
-            /* Call it */
+            /* Check for highest-level device completion routines */
+            if (Irp->CurrentLocation == (Irp->StackCount + 1))
+            {
+                /* Clear the DO, since the current stack location is invalid */
+                DeviceObject = NULL;
+            }
+            else
+            {
+                /* Otherwise, return the real one */
+                DeviceObject = IoGetCurrentIrpStackLocation(Irp)->DeviceObject;
+            }
+
+            /* Call the completion routine */
             Status = StackPtr->CompletionRoutine(DeviceObject,
                                                  Irp,
                                                  StackPtr->Context);
 
-            /* Don't touch the Packet if this was returned. It might be gone! */
+            /* Don't touch the Packet in this case, since it might be gone! */
             if (Status == STATUS_MORE_PROCESSING_REQUIRED) return;
         }
         else
         {
-            if ((Irp->CurrentLocation <= Irp->StackCount) && (Irp->PendingReturned))
-            {
+            /* Otherwise, check if this is a completed IRP */
+            if ((Irp->CurrentLocation <= Irp->StackCount) &&
+                (Irp->PendingReturned))
+            {
+                /* Mark it as pending */
                 IoMarkIrpPending(Irp);
             }
         }
 
-        /* Move to next stack */
+        /* Move to next stack location and pointer */
         IoSkipCurrentIrpStackLocation(Irp);
-    }
-
-    /* Windows NT File System Internals, page 165 */
+        StackPtr++;
+    } while (Irp->CurrentLocation <= (Irp->StackCount + 1));
+
+    /* Check if the IRP is an associated IRP */
     if (Irp->Flags & IRP_ASSOCIATED_IRP)
     {
-        ULONG MasterIrpCount;
-        PIRP MasterIrp = Irp->AssociatedIrp.MasterIrp;
-
         /* This should never happen! */
         ASSERT(IsListEmpty(&Irp->ThreadListEntry));
 
-        /* Decrement and get the old count */
-        MasterIrpCount = InterlockedDecrement(&MasterIrp->AssociatedIrp.IrpCount);
-
-        /* Free MDLs and IRP */
+        /* Get the master IRP and count */
+        MasterIrp = Irp->AssociatedIrp.MasterIrp;
+        MasterIrpCount = InterlockedDecrement(&MasterIrp->
+                                              AssociatedIrp.IrpCount);
+
+        /* Set the thread of this IRP as the master's */
+        Irp->Tail.Overlay.Thread = MasterIrp->Tail.Overlay.Thread;
+
+        /* Free the MDLs */
         while ((Mdl = Irp->MdlAddress))
         {
+            /* Go to the next one */
             Irp->MdlAddress = Mdl->Next;
             IoFreeMdl(Mdl);
         }
+
+        /* Free the IRP itself */
         IoFreeIrp(Irp);
 
         /* Complete the Master IRP */
@@ -1123,8 +1069,16 @@
         return;
     }
 
-    /* Windows NT File System Internals, page 165 */
-    if (Irp->Flags & (IRP_PAGING_IO|IRP_CLOSE_OPERATION))
+    /* Check if we have an auxiliary buffer */
+    if (Irp->Tail.Overlay.AuxiliaryBuffer)
+    {
+        /* Free it */
+        ExFreePool(Irp->Tail.Overlay.AuxiliaryBuffer);
+        Irp->Tail.Overlay.AuxiliaryBuffer = NULL;
+    }
+
+    /* Check if this is a Paging I/O or Close Operation */
+    if (Irp->Flags & (IRP_PAGING_IO | IRP_CLOSE_OPERATION))
     {
         /* This should never happen! */
         ASSERT(IsListEmpty(&Irp->ThreadListEntry));
@@ -1137,10 +1091,7 @@
             KeSetEvent(Irp->UserEvent, PriorityBoost, FALSE);
 
             /* Free the IRP for a Paging I/O Only, Close is handled by us */
-            if (Irp->Flags & IRP_SYNCHRONOUS_PAGING_IO)
-            {
-                IoFreeIrp(Irp);
-            }
+            if (Irp->Flags & IRP_SYNCHRONOUS_PAGING_IO) IoFreeIrp(Irp);
         }
         else
         {
@@ -1163,6 +1114,8 @@
             ASSERT(FALSE);
 #endif
         }
+
+        /* Get out of here */
         return;
     }
 
@@ -1175,22 +1128,33 @@
     }
 
     /* Check if we should exit because of a Deferred I/O (page 168) */
-    if (Irp->Flags & IRP_DEFER_IO_COMPLETION && !Irp->PendingReturned)
-    {
+    if ((Irp->Flags & IRP_DEFER_IO_COMPLETION) && !(Irp->PendingReturned))
+    {
+        /*
+         * Return without queuing the completion APC, since the caller will
+         * take care of doing its own optimized completion at PASSIVE_LEVEL.
+         */
         return;
     }
 
-    /* Now queue the special APC */
+    /* Get the thread and file object */
+    Thread = Irp->Tail.Overlay.Thread;
+    FileObject = Irp->Tail.Overlay.OriginalFileObject;
+
+    /* Make sure the IRP isn't cancelled */
     if (!Irp->Cancel)
     {
+        /* Initialize the APC */
         KeInitializeApc(&Irp->Tail.Apc,
                         &Thread->Tcb,
                         Irp->ApcEnvironment,
                         IopCompleteRequest,
                         NULL,
-                        (PKNORMAL_ROUTINE) NULL,
+                        NULL,
                         KernelMode,
                         NULL);
+
+        /* Queue it */
         KeInsertQueueApc(&Irp->Tail.Apc,
                          FileObject,
                          NULL, /* This is used for REPARSE stuff */
@@ -1199,17 +1163,20 @@
     else
     {
         /* The IRP just got cancelled... does a thread still own it? */
-        if ((Thread = Irp->Tail.Overlay.Thread))
-        {
-            /* Yes! There is still hope! */
+        Thread = Irp->Tail.Overlay.Thread;
+        if (Thread)
+        {
+            /* Yes! There is still hope! Initialize the APC */
             KeInitializeApc(&Irp->Tail.Apc,
                             &Thread->Tcb,
                             Irp->ApcEnvironment,
                             IopCompleteRequest,
                             NULL,
-                            (PKNORMAL_ROUTINE) NULL,
+                            NULL,
                             KernelMode,
                             NULL);
+
+            /* Queue it */
             KeInsertQueueApc(&Irp->Tail.Apc,
                              FileObject,
                              NULL, /* This is used for REPARSE stuff */




More information about the Ros-diffs mailing list