Re: [ros-dev] [ros-diffs] [sginsberg] 35565: - In IoAcquire/ReleaseCancelSpinLock, use the queued "IopCancelSpinLock" instead of a ros-specific "CancelSpinLock" standard spinlock - Misc fixes

[Alex Ionescu]

Please don't write formatting fixes as "Misc Fixes". It's a pain for  
reviewers who expect actual functionality fixes, and also for  
changelog writers, which will try to understand what exactly you fixed.

Furthermore, such formatting patches should have [FORMATTING] attached  
to them.

And finally, formatting patches should not be mixed with code patches.

I know you're new to trunk, but perhaps you should read over the rules  
first.

I appreciate your patches, but you have a bit of a cowboy attitude  
with your commits, which is bound to hurt someone in the end.

On 23-Aug-08, at 9:30 AM, sginsberg(a)svn.reactos.org wrote:

>   Author: sginsberg
>  Date: Sat Aug 23 11:30:14 2008
>  New Revision: 35565
>
>  URL: http://svn.reactos.org/svn/reactos?rev=35565&view=rev
>  Log:
>  - In IoAcquire/ReleaseCancelSpinLock, use the queued  
>  "IopCancelSpinLock" instead of a ros-specific "CancelSpinLock"  
>  standard spinlock
>  - Misc fixes
>
>  Modified:
>     trunk/reactos/ntoskrnl/io/iomgr/error.c
>     trunk/reactos/ntoskrnl/io/iomgr/iofunc.c
>     trunk/reactos/ntoskrnl/io/iomgr/iomgr.c
>     trunk/reactos/ntoskrnl/io/iomgr/irp.c
>     trunk/reactos/ntoskrnl/io/iomgr/util.c
>
>  Modified: trunk/reactos/ntoskrnl/io/iomgr/error.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/iomgr/error.c?…
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  ======================================================================
>  --- trunk/reactos/ntoskrnl/io/iomgr/error.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/io/iomgr/error.c [iso-8859-1] Sat Aug 23  
>  11:30:14 2008
>  @@ -122,7 +122,7 @@
>                             NULL);
>      if (NT_SUCCESS(Status))
>      {
>  -        /* Remmeber we're connected */
>  +        /* Remember we're connected */
>          IopLogPortConnected = TRUE;
>          return TRUE;
>      }
>  @@ -583,7 +583,7 @@
>      KeAcquireSpinLock(&IopLogListLock, &Irql);
>      InsertHeadList(&IopErrorLogListHead, &LogEntry->ListEntry);
>
>  -    /* Check if the worker is runnign */
>  +    /* Check if the worker is running */
>      if (!IopLogWorkerRunning)
>      {
>  #if 0
>  @@ -648,7 +648,7 @@
>                                IN PKTHREAD Thread)
>  {
>      UNIMPLEMENTED;
>  -    return(FALSE);
>  +    return FALSE;
>  }
>
>  /*
>  @@ -659,14 +659,12 @@
>  IoSetThreadHardErrorMode(IN BOOLEAN HardErrorEnabled)
>  {
>      PETHREAD Thread = PsGetCurrentThread();
>  -    BOOLEAN Old;
>  +    BOOLEAN OldMode;
>
>      /* Get the current value */
>  -    Old = !Thread->HardErrorsAreDisabled;
>  +    OldMode = !Thread->HardErrorsAreDisabled;
>
>      /* Set the new one and return the old */
>      Thread->HardErrorsAreDisabled = !HardErrorEnabled;
>  -    return Old;
>  -}
>  -
>  -/* EOF */
>  +    return OldMode;
>  +}
>
>  Modified: trunk/reactos/ntoskrnl/io/iomgr/iofunc.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/iomgr/iofunc.c…
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  ======================================================================
>  --- trunk/reactos/ntoskrnl/io/iomgr/iofunc.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/io/iomgr/iofunc.c [iso-8859-1] Sat Aug 23  
>  11:30:14 2008
>  @@ -682,7 +682,7 @@
>      StackPtr->FileObject = FileObject;
>
>      /* Call the Driver */
>  -    return IofCallDriver(DeviceObject, Irp);
>  +    return IoCallDriver(DeviceObject, Irp);
>  }
>
>  /*
>  @@ -732,7 +732,7 @@
>      StackPtr->FileObject = FileObject;
>
>      /* Call the Driver */
>  -    return IofCallDriver(DeviceObject, Irp);
>  +    return IoCallDriver(DeviceObject, Irp);
>  }
>
>  /*
>  @@ -776,7 +776,7 @@
>  }
>
>  /*
>  - * @unimplemented
>  + * @implemented
>   */
>  NTSTATUS
>  NTAPI
>  @@ -2045,7 +2045,7 @@
>              CapturedByteOffset = FileObject->CurrentByteOffset;
>          }
>
>  -        /* Rememer we are sync */
>  +        /* Remember we are sync */
>          Synchronous = TRUE;
>      }
>      else if (!(ByteOffset) &&
>  @@ -2904,7 +2904,7 @@
>              CapturedByteOffset = FileObject->CurrentByteOffset;
>          }
>
>  -        /* Rememer we are sync */
>  +        /* Remember we are sync */
>          Synchronous = TRUE;
>      }
>      else if (!(ByteOffset) &&
>
>  Modified: trunk/reactos/ntoskrnl/io/iomgr/iomgr.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/iomgr/iomgr.c?…
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  ======================================================================
>  --- trunk/reactos/ntoskrnl/io/iomgr/iomgr.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/io/iomgr/iomgr.c [iso-8859-1] Sat Aug 23  
>  11:30:14 2008
>  @@ -68,7 +68,6 @@
>  extern LIST_ENTRY IopTimerQueueHead;
>  extern KDPC IopTimerDpc;
>  extern KTIMER IopTimer;
>  -extern KSPIN_LOCK CancelSpinLock;
>  extern KSPIN_LOCK IoVpbLock;
>  extern KSPIN_LOCK IoStatisticsLock;
>  extern KSPIN_LOCK DriverReinitListLock;
>  @@ -463,7 +462,6 @@
>      InitializeListHead(&LastChanceShutdownListHead);
>      InitializeListHead(&FsChangeNotifyListHead);
>      InitializeListHead(&IopErrorLogListHead);
>  -    KeInitializeSpinLock(&CancelSpinLock);
>      KeInitializeSpinLock(&IoVpbLock);
>      KeInitializeSpinLock(&IoStatisticsLock);
>      KeInitializeSpinLock(&DriverReinitListLock);
>
>  Modified: trunk/reactos/ntoskrnl/io/iomgr/irp.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/iomgr/irp.c?re…
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  ======================================================================
>  --- trunk/reactos/ntoskrnl/io/iomgr/irp.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/io/iomgr/irp.c [iso-8859-1] Sat Aug 23  
>  11:30:14 2008
>  @@ -1,8 +1,7 @@
>  -
>  /*
>   * PROJECT:         ReactOS Kernel
>   * LICENSE:         GPL - See COPYING in the top level directory
>  - * FILE:            ntoskrnl/io/irp.c
>  + * FILE:            ntoskrnl/io/iomgr/irp.c
>   * PURPOSE:         IRP Handling Functions
>   * PROGRAMMERS:     Alex Ionescu (alex.ionescu(a)reactos.org)
>   *                  Gunnar Dalsnes
>  @@ -627,7 +626,7 @@
>
>      /* Allocate IRP */
>      Irp = IoAllocateIrp(DeviceObject->StackSize, FALSE);
>  -    if (!Irp) return Irp;
>  +    if (!Irp) return NULL;
>
>      /* Get the Stack */
>      StackPtr = IoGetNextIrpStackLocation(Irp);
>  @@ -731,7 +730,7 @@
>      Irp->UserIosb = IoStatusBlock;
>      Irp->Tail.Overlay.Thread = PsGetCurrentThread();
>
>  -    /* Set the Status Block after all is done */
>  +    /* Return the IRP */
>      IOTRACE(IO_IRP_DEBUG,
>              "%s - Built IRP %p with Major, Buffer, DO %lx %p %p\n",
>              __FUNCTION__,
>  @@ -763,7 +762,7 @@
>
>      /* Allocate IRP */
>      Irp = IoAllocateIrp(DeviceObject->StackSize, FALSE);
>  -    if (!Irp) return Irp;
>  +    if (!Irp) return NULL;
>
>      /* Get the Stack */
>      StackPtr = IoGetNextIrpStackLocation(Irp);
>  @@ -857,6 +856,7 @@
>              }
>              else
>              {
>  +                /* Clear the flags */
>                  Irp->Flags = 0;
>              }
>
>  @@ -1058,7 +1058,12 @@
>           * Don't stay here forever if some broken driver doesn't  
>  complete
>           * the IRP.
>           */
>  -        if (!(Retries--)) IopRemoveThreadIrp();
>  +        if (!(Retries--))
>  +        {
>  +            /* Print out a message and remove the IRP */
>  +            DPRINT1("Broken driver did not complete!\n");
>  +            IopRemoveThreadIrp();
>  +        }
>
>          /* Raise the IRQL Again */
>          KeRaiseIrql(APC_LEVEL, &OldIrql);
>  @@ -1076,7 +1081,7 @@
>  IoCallDriver(IN PDEVICE_OBJECT DeviceObject,
>               IN PIRP Irp)
>  {
>  -    /* Call fast call */
>  +    /* Call fastcall */
>      return IofCallDriver(DeviceObject, Irp);
>  }
>
>  @@ -1112,7 +1117,7 @@
>                IN PIRP Irp)
>  {
>      PDRIVER_OBJECT DriverObject;
>  -    PIO_STACK_LOCATION Param;
>  +    PIO_STACK_LOCATION StackPtr;
>
>      /* Get the Driver Object */
>      DriverObject = DeviceObject->DriverObject;
>  @@ -1126,15 +1131,15 @@
>      }
>
>      /* Now update the stack location */
>  -    Param = IoGetNextIrpStackLocation(Irp);
>  -    Irp->Tail.Overlay.CurrentStackLocation = Param;
>  +    StackPtr = IoGetNextIrpStackLocation(Irp);
>  +    Irp->Tail.Overlay.CurrentStackLocation = StackPtr;
>
>      /* Get the Device Object */
>  -    Param->DeviceObject = DeviceObject;
>  +    StackPtr->DeviceObject = DeviceObject;
>
>      /* Call it */
>  -    return DriverObject->MajorFunction[Param->MajorFunction] 
>  (DeviceObject,
>  -                                                             Irp);
>  +    return DriverObject->MajorFunction[StackPtr->MajorFunction] 
>  (DeviceObject,
>  +                                                                Irp);
>  }
>
>  FORCEINLINE
>  @@ -1417,10 +1422,9 @@
>
>  NTSTATUS
>  NTAPI
>  -IopSynchronousCompletion(
>  -    IN PDEVICE_OBJECT DeviceObject,
>  -    IN PIRP Irp,
>  -    IN PVOID Context)
>  +IopSynchronousCompletion(IN PDEVICE_OBJECT DeviceObject,
>  +                         IN PIRP Irp,
>  +                         IN PVOID Context)
>  {
>      if (Irp->PendingReturned)
>          KeSetEvent((PKEVENT)Context, IO_NO_INCREMENT, FALSE);
>  @@ -1464,6 +1468,7 @@
>          KeWaitForSingleObject(&Event, Suspended, KernelMode, FALSE,  
>  NULL);
>      }
>
>  +    /* Return success */
>      return TRUE;
>  }
>
>  @@ -1550,10 +1555,12 @@
>  /*
>   * @implemented
>   */
>  -PEPROCESS NTAPI
>  +PEPROCESS
>  +NTAPI
>  IoGetRequestorProcess(IN PIRP Irp)
>  {
>  -    return(Irp->Tail.Overlay.Thread->ThreadsProcess);
>  +    /* Return the requestor process */
>  +    return Irp->Tail.Overlay.Thread->ThreadsProcess;
>  }
>
>  /*
>  @@ -1563,6 +1570,7 @@
>  NTAPI
>  IoGetRequestorProcessId(IN PIRP Irp)
>  {
>  +    /* Return the requestor process' id */
>      return (ULONG)(IoGetRequestorProcess(Irp)->UniqueProcessId);
>  }
>
>  @@ -1586,6 +1594,7 @@
>  NTAPI
>  IoGetTopLevelIrp(VOID)
>  {
>  +    /* Return the IRP */
>      return (PIRP)PsGetCurrentThread()->TopLevelIrp;
>  }
>
>  @@ -1736,5 +1745,3 @@
>      /* Set the IRP */
>      PsGetCurrentThread()->TopLevelIrp = (ULONG)Irp;
>  }
>  -
>  -/* EOF */
>
>  Modified: trunk/reactos/ntoskrnl/io/iomgr/util.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/iomgr/util.c?r…
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  ======================================================================
>  --- trunk/reactos/ntoskrnl/io/iomgr/util.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/io/iomgr/util.c [iso-8859-1] Sat Aug 23  
>  11:30:14 2008
>  @@ -1,9 +1,11 @@
>  /*
>   * PROJECT:         ReactOS Kernel
>   * LICENSE:         GPL - See COPYING in the top level directory
>  - * FILE:            ntoskrnl/io/util.c
>  + * FILE:            ntoskrnl/io/iomgr/util.c
>   * PURPOSE:         I/O Utility Functions
>  - * PROGRAMMERS:     <UNKNOWN>
>  + * PROGRAMMERS:     Alex Ionescu (alex.ionescu(a)reactos.org)
>  + *                  Aleksey Bragin (aleksey(a)reactos.org)
>  + *                  Daniel Zimmerman (netzimme(a)aim.com)
>   */
>
>  /* INCLUDES  
>  *****************************************************************/
>  @@ -17,10 +19,6 @@
>  RtlpGetStackLimits(PULONG_PTR StackBase,
>                     PULONG_PTR StackLimit);
>
>  -/* DATA  
>  **********************************************************************/
>  -
>  -KSPIN_LOCK CancelSpinLock;
>  -
>  /* FUNCTIONS  
>  *****************************************************************/
>
>  /*
>  @@ -28,10 +26,10 @@
>   */
>  VOID
>  NTAPI
>  -IoAcquireCancelSpinLock(PKIRQL Irql)
>  +IoAcquireCancelSpinLock(OUT PKIRQL Irql)
>  {
>      /* Just acquire the internal lock */
>  -    KeAcquireSpinLock(&CancelSpinLock,Irql);
>  +    *Irql = KeAcquireQueuedSpinLock(LockQueueIoCancelLock);
>  }
>
>  /*
>  @@ -113,6 +111,7 @@
>  NTAPI
>  IoGetCurrentProcess(VOID)
>  {
>  +    /* Return the current thread's process */
>      return (PEPROCESS)PsGetCurrentThread()->Tcb.ApcState.Process;
>  }
>
>  @@ -121,10 +120,10 @@
>   */
>  VOID
>  NTAPI
>  -IoReleaseCancelSpinLock(KIRQL Irql)
>  +IoReleaseCancelSpinLock(IN KIRQL Irql)
>  {
>      /* Release the internal lock */
>  -    KeReleaseSpinLock(&CancelSpinLock,Irql);
>  +    KeReleaseQueuedSpinLock(LockQueueIoCancelLock, Irql);
>  }
>
>  /*
>  @@ -315,4 +314,3 @@
>      UNIMPLEMENTED;
>      return STATUS_NOT_IMPLEMENTED;
>  }
>  -/* EOF */
>  
Best regards,
Alex Ionescu

[Timo Kreuzer]

Why this?
>  >
> ======================================================================
> > --- trunk/reactos/ntoskrnl/io/iomgr/iofunc.c [iso-8859-1] (original)
> > +++ trunk/reactos/ntoskrnl/io/iomgr/iofunc.c [iso-8859-1] Sat Aug 23  
> > 11:30:14 2008
> > @@ -682,7 +682,7 @@
> >     StackPtr->FileObject = FileObject;
> >
> >     /* Call the Driver */
> > -    return IofCallDriver(DeviceObject, Irp);
> > +    return IoCallDriver(DeviceObject, Irp);
> > }
> >
> > /*
> > @@ -732,7 +732,7 @@
> >     StackPtr->FileObject = FileObject;
> >
> >     /* Call the Driver */
> > -    return IofCallDriver(DeviceObject, Irp);
> > +    return IoCallDriver(DeviceObject, Irp);
> > }
> >
> >