[hbelusca] 58534: [SERVICES] - It seems (after testing) that services report now correctly their state to the SCM. So we can start them in SERVICE_START_PENDING state (see revisions r45626 and r456...

Author: hbelusca
Date: Sun Mar 17 12:39:44 2013
New Revision: 58534

URL: http://svn.reactos.org/svn/reactos?rev=58534&view=rev
Log:
[SERVICES]
- It seems (after testing) that services report now correctly their state to the SCM. So
we can start them in SERVICE_START_PENDING state (see revisions r45626 and r45640).
- Add some informative comments.
- Use a helper function to create start events at initialization time.
- When autostart services are up, signal an event. (see revisions r45633 and r45658).
- Wait for LSASS just after having created the services database, and before calling
ScmGetBootAndSystemDriverState (conform to Windows Internals 4th, page 224).

---------

- When starting auto-start services, hold a lock during all the operation in such a way
that, if an external program wants to start a service, it is obliged to wait till all the
auto-start services have been started (usual service starting also uses that lock).

CORE-7001 #resolve #comment Should be fixed by r58534. Do not hesitate to reopen this
bug-report if the problem reappears.

Modified:
    trunk/reactos/base/system/services/database.c
    trunk/reactos/base/system/services/rpcserver.c
    trunk/reactos/base/system/services/services.c
    trunk/reactos/base/system/services/services.h

Modified: trunk/reactos/base/system/services/database.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/system/services/datab…
==============================================================================
--- trunk/reactos/base/system/services/database.c [iso-8859-1] (original)
+++ trunk/reactos/base/system/services/database.c [iso-8859-1] Sun Mar 17 12:39:44 2013
@@ -19,9 +19,9 @@
 
 /*
  * Uncomment the line below to start services
- *  using the SERVICE_START_PENDING state
+ * using the SERVICE_START_PENDING state.
  */
-// #define USE_SERVICE_START_PENDING
+#define USE_SERVICE_START_PENDING
 
 /*
  * Uncomment the line below to use asynchronous IO operations
@@ -38,6 +38,7 @@
 static RTL_RESOURCE DatabaseLock;
 static DWORD ResumeCount = 1;
 
+/* The critical section synchronizes service controls commands */
 static CRITICAL_SECTION ControlServiceCriticalSection;
 static DWORD PipeTimeout = 30000; /* 30 Seconds */
 
@@ -359,7 +360,7 @@
 
 DWORD
 ScmCreateNewServiceRecord(LPCWSTR lpServiceName,
-                          PSERVICE *lpServiceRecord)
+                          PSERVICE* lpServiceRecord)
 {
     PSERVICE lpService = NULL;
 
@@ -772,13 +773,11 @@
 
     if (Service->Status.dwServiceType == SERVICE_KERNEL_DRIVER)
     {
-        RtlInitUnicodeString(&DirName,
-                             L"\\Driver");
-    }
-    else
-    {
-        RtlInitUnicodeString(&DirName,
-                             L"\\FileSystem");
+        RtlInitUnicodeString(&DirName, L"\\Driver");
+    }
+    else // if (Service->Status.dwServiceType == SERVICE_FILE_SYSTEM_DRIVER)
+    {
+        RtlInitUnicodeString(&DirName, L"\\FileSystem");
     }
 
     InitializeObjectAttributes(&ObjectAttributes,
@@ -796,7 +795,7 @@
     }
 
     BufferLength = sizeof(OBJECT_DIRECTORY_INFORMATION) +
-                   2 * MAX_PATH * sizeof(WCHAR);
+                       2 * MAX_PATH * sizeof(WCHAR);
     DirInfo = HeapAlloc(GetProcessHeap(),
                         HEAP_ZERO_MEMORY,
                         BufferLength);
@@ -1096,7 +1095,7 @@
 static DWORD
 ScmSendStartCommand(PSERVICE Service,
                     DWORD argc,
-                    LPWSTR *argv)
+                    LPWSTR* argv)
 {
     PSCM_CONTROL_PACKET ControlPacket;
     SCM_REPLY_PACKET ReplyPacket;
@@ -1496,7 +1495,7 @@
 static DWORD
 ScmStartUserModeService(PSERVICE Service,
                         DWORD argc,
-                        LPWSTR *argv)
+                        LPWSTR* argv)
 {
     PROCESS_INFORMATION ProcessInformation;
     STARTUPINFOW StartupInfo;
@@ -1512,6 +1511,7 @@
         return ScmSendStartCommand(Service, argc, argv);
     }
 
+    /* Otherwise start its process */
     ZeroMemory(&StartupInfo, sizeof(StartupInfo));
     StartupInfo.cb = sizeof(StartupInfo);
     ZeroMemory(&ProcessInformation, sizeof(ProcessInformation));
@@ -1551,9 +1551,7 @@
     if (dwError == ERROR_SUCCESS)
     {
         /* Send start command */
-        dwError = ScmSendStartCommand(Service,
-                                      argc,
-                                      argv);
+        dwError = ScmSendStartCommand(Service, argc, argv);
     }
     else
     {
@@ -1569,24 +1567,22 @@
 }
 
 
-DWORD
-ScmStartService(PSERVICE Service, DWORD argc, LPWSTR *argv)
+static DWORD
+ScmLoadService(PSERVICE Service,
+               DWORD argc,
+               LPWSTR* argv)
 {
     PSERVICE_GROUP Group = Service->lpGroup;
     DWORD dwError = ERROR_SUCCESS;
     LPCWSTR ErrorLogStrings[2];
     WCHAR szErrorBuffer[32];
 
-    DPRINT("ScmStartService() called\n");
-
+    DPRINT("ScmLoadService() called\n");
     DPRINT("Start Service %p (%S)\n", Service, Service->lpServiceName);
 
-    EnterCriticalSection(&ControlServiceCriticalSection);
-
     if (Service->Status.dwCurrentState != SERVICE_STOPPED)
     {
         DPRINT("Service %S is already running!\n", Service->lpServiceName);
-        LeaveCriticalSection(&ControlServiceCriticalSection);
         return ERROR_SERVICE_ALREADY_RUNNING;
     }
 
@@ -1602,7 +1598,7 @@
             Service->Status.dwCurrentState = SERVICE_RUNNING;
         }
     }
-    else
+    else // if (Service->Status.dwServiceType & (SERVICE_WIN32 |
SERVICE_INTERACTIVE_PROCESS))
     {
         /* Start user-mode service */
         dwError = ScmCreateOrReferenceServiceImage(Service);
@@ -1625,9 +1621,7 @@
         }
     }
 
-    LeaveCriticalSection(&ControlServiceCriticalSection);
-
-    DPRINT("ScmStartService() done (Error %lu)\n", dwError);
+    DPRINT("ScmLoadService() done (Error %lu)\n", dwError);
 
     if (dwError == ERROR_SUCCESS)
     {
@@ -1677,17 +1671,80 @@
 }
 
 
+DWORD
+ScmStartService(PSERVICE Service,
+                DWORD argc,
+                LPWSTR* argv)
+{
+    DWORD dwError = ERROR_SUCCESS;
+    SC_RPC_LOCK Lock = NULL;
+
+    DPRINT("ScmStartService() called\n");
+    DPRINT("Start Service %p (%S)\n", Service, Service->lpServiceName);
+
+    /* Acquire the service control critical section, to synchronize starts */
+    EnterCriticalSection(&ControlServiceCriticalSection);
+
+    /*
+     * Acquire the user service start lock while the service is starting, if
+     * needed (i.e. if we are not starting it during the initialization phase).
+     * If we don't success, bail out.
+     */
+    if (!ScmInitialize)
+    {
+        dwError = ScmAcquireServiceStartLock(TRUE, &Lock);
+        if (dwError != ERROR_SUCCESS) goto done;
+    }
+
+    /* Really start the service */
+    dwError = ScmLoadService(Service, argc, argv);
+
+    /* Release the service start lock, if needed, and the critical section */
+    if (Lock) ScmReleaseServiceStartLock(&Lock);
+
+done:
+    LeaveCriticalSection(&ControlServiceCriticalSection);
+
+    DPRINT("ScmStartService() done (Error %lu)\n", dwError);
+
+    return dwError;
+}
+
+
 VOID
 ScmAutoStartServices(VOID)
 {
+    DWORD dwError = ERROR_SUCCESS;
+    SC_RPC_LOCK Lock = NULL;
+
     PLIST_ENTRY GroupEntry;
     PLIST_ENTRY ServiceEntry;
     PSERVICE_GROUP CurrentGroup;
     PSERVICE CurrentService;
     WCHAR szSafeBootServicePath[MAX_PATH];
-    DWORD dwError;
     HKEY hKey;
     ULONG i;
+
+    /* Acquire the service control critical section, to synchronize starts */
+    EnterCriticalSection(&ControlServiceCriticalSection);
+
+    /*
+     * Acquire the user service start lock while the service is starting, if
+     * needed (i.e. if we are not starting it during the initialization phase).
+     * If we don't success, bail out.
+     */
+    if (!ScmInitialize)
+    {
+        /*
+         * Actually this code is never executed since we are called
+         * at initialization time, so that ScmInitialize == TRUE.
+         * But keep the code here if someday we are called later on
+         * for whatever reason...
+         */
+        dwError = ScmAcquireServiceStartLock(TRUE, &Lock);
+        if (dwError != ERROR_SUCCESS) goto done;
+    }
+
 
     /* Clear 'ServiceVisited' flag (or set if not to start in Safe Mode) */
     ServiceEntry = ServiceListHead.Flink;
@@ -1779,7 +1836,7 @@
                     (CurrentService->dwTag == CurrentGroup->TagArray[i]))
                 {
                     CurrentService->ServiceVisited = TRUE;
-                    ScmStartService(CurrentService, 0, NULL);
+                    ScmLoadService(CurrentService, 0, NULL);
                 }
 
                 ServiceEntry = ServiceEntry->Flink;
@@ -1797,7 +1854,7 @@
                 (CurrentService->ServiceVisited == FALSE))
             {
                 CurrentService->ServiceVisited = TRUE;
-                ScmStartService(CurrentService, 0, NULL);
+                ScmLoadService(CurrentService, 0, NULL);
             }
 
             ServiceEntry = ServiceEntry->Flink;
@@ -1817,7 +1874,7 @@
             (CurrentService->ServiceVisited == FALSE))
         {
             CurrentService->ServiceVisited = TRUE;
-            ScmStartService(CurrentService, 0, NULL);
+            ScmLoadService(CurrentService, 0, NULL);
         }
 
         ServiceEntry = ServiceEntry->Flink;
@@ -1834,7 +1891,7 @@
             (CurrentService->ServiceVisited == FALSE))
         {
             CurrentService->ServiceVisited = TRUE;
-            ScmStartService(CurrentService, 0, NULL);
+            ScmLoadService(CurrentService, 0, NULL);
         }
 
         ServiceEntry = ServiceEntry->Flink;
@@ -1848,6 +1905,13 @@
         CurrentService->ServiceVisited = FALSE;
         ServiceEntry = ServiceEntry->Flink;
     }
+
+
+    /* Release the service start lock, if needed, and the critical section */
+    if (Lock) ScmReleaseServiceStartLock(&Lock);
+
+done:
+    LeaveCriticalSection(&ControlServiceCriticalSection);
 }
 
 

Modified: trunk/reactos/base/system/services/rpcserver.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/system/services/rpcse…
==============================================================================
--- trunk/reactos/base/system/services/rpcserver.c [iso-8859-1] (original)
+++ trunk/reactos/base/system/services/rpcserver.c [iso-8859-1] Sun Mar 17 12:39:44 2013
@@ -2895,7 +2895,6 @@
     DWORD dwError = ERROR_SUCCESS;
     PSERVICE_HANDLE hSvc;
     PSERVICE lpService = NULL;
-    SC_RPC_LOCK Lock = NULL;
 
 #ifndef NDEBUG
     DWORD i;
@@ -2941,16 +2940,8 @@
     if (lpService->bDeleted)
         return ERROR_SERVICE_MARKED_FOR_DELETE;
 
-    /* Acquire the service start lock until the service has been started */
-    dwError = ScmAcquireServiceStartLock(TRUE, &Lock);
-    if (dwError != ERROR_SUCCESS)
-        return dwError;
-
     /* Start the service */
     dwError = ScmStartService(lpService, argc, (LPWSTR*)argv);
-
-    /* Release the service start lock */
-    ScmReleaseServiceStartLock(&Lock);
 
     return dwError;
 }
@@ -4171,7 +4162,6 @@
     DWORD dwError = ERROR_SUCCESS;
     PSERVICE_HANDLE hSvc;
     PSERVICE lpService = NULL;
-    SC_RPC_LOCK Lock = NULL;
     LPWSTR *lpVector = NULL;
     DWORD i;
     DWORD dwLength;
@@ -4244,16 +4234,8 @@
         }
     }
 
-    /* Acquire the service start lock until the service has been started */
-    dwError = ScmAcquireServiceStartLock(TRUE, &Lock);
-    if (dwError != ERROR_SUCCESS)
-        goto done;
-
     /* Start the service */
     dwError = ScmStartService(lpService, argc, lpVector);
-
-     /* Release the service start lock */
-     ScmReleaseServiceStartLock(&Lock);
 
 done:
     /* Free the Unicode argument vector */

Modified: trunk/reactos/base/system/services/services.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/system/services/servi…
==============================================================================
--- trunk/reactos/base/system/services/services.c [iso-8859-1] (original)
+++ trunk/reactos/base/system/services/services.c [iso-8859-1] Sun Mar 17 12:39:44 2013
@@ -22,6 +22,12 @@
 #define PIPE_BUFSIZE 1024
 #define PIPE_TIMEOUT 1000
 
+/* Defined in include/reactos/services/services.h */
+// #define SCM_START_EVENT             L"SvcctrlStartEvent_A3752DX"
+#define SCM_AUTOSTARTCOMPLETE_EVENT L"SC_AutoStartComplete"
+#define LSA_RPC_SERVER_ACTIVE       L"LSA_RPC_SERVER_ACTIVE"
+
+BOOL ScmInitialize = FALSE;
 BOOL ScmShutdown = FALSE;
 static HANDLE hScmShutdownEvent = NULL;
 
@@ -77,35 +83,38 @@
 
 
 BOOL
-ScmCreateStartEvent(PHANDLE StartEvent)
-{
+ScmCreateControlEvent(PHANDLE Event,
+                      LPCWSTR Name,
+                      DWORD dwDesiredAccess)
+{
+    /*
+     * This function creates a generic non-inheritable event
+     * and return a handle to the caller. The caller must
+     * close this handle afterwards.
+     */
+
     HANDLE hEvent;
 
-    hEvent = CreateEventW(NULL,
-                          TRUE,
-                          FALSE,
-                          L"SvcctrlStartEvent_A3752DX");
+    hEvent = CreateEventW(NULL, TRUE, FALSE, Name);
     if (hEvent == NULL)
     {
         if (GetLastError() == ERROR_ALREADY_EXISTS)
         {
-            hEvent = OpenEventW(EVENT_ALL_ACCESS,
-                                FALSE,
-                                L"SvcctrlStartEvent_A3752DX");
-            if (hEvent == NULL)
-            {
-                return FALSE;
-            }
+            hEvent = OpenEventW(dwDesiredAccess, FALSE, Name);
         }
-        else
-        {
-            return FALSE;
-        }
-    }
-
-    *StartEvent = hEvent;
-
-    return TRUE;
+    }
+
+    if (hEvent)
+    {
+        DPRINT("SERVICES: Created event %S with handle %x\n", Name, hEvent);
+        *Event = hEvent;
+        return TRUE;
+    }
+    else
+    {
+        DPRINT1("SERVICES: Failed to create event %S (Error %lu)\n", Name,
GetLastError());
+        return FALSE;
+    }
 }
 
 
@@ -113,35 +122,21 @@
 ScmWaitForLsa(VOID)
 {
     HANDLE hEvent;
-    DWORD dwError;
-
-    hEvent = CreateEventW(NULL,
-                          TRUE,
-                          FALSE,
-                          L"LSA_RPC_SERVER_ACTIVE");
-    if (hEvent == NULL)
-    {
-        dwError = GetLastError();
-        DPRINT1("Failed to create the notication event (Error %lu)\n",
dwError);
-
-        if (dwError == ERROR_ALREADY_EXISTS)
-        {
-            hEvent = OpenEventW(SYNCHRONIZE,
-                                FALSE,
-                                L"LSA_RPC_SERVER_ACTIVE");
-            if (hEvent == NULL)
-            {
-               DPRINT1("Could not open the notification event (Error %lu)\n",
GetLastError());
-               return;
-            }
-        }
-    }
-
-    DPRINT("Wait for the LSA server!\n");
-    WaitForSingleObject(hEvent, INFINITE);
-    DPRINT("LSA server running!\n");
-
-    CloseHandle(hEvent);
+
+    if (!ScmCreateControlEvent(&hEvent,
+                               LSA_RPC_SERVER_ACTIVE,
+                               SYNCHRONIZE))
+    {
+        DPRINT1("Failed to create the notification event (Error %lu)\n",
GetLastError());
+    }
+    else
+    {
+        DPRINT("Wait for the LSA server!\n");
+        WaitForSingleObject(hEvent, INFINITE);
+        DPRINT("LSA server running!\n");
+
+        CloseHandle(hEvent);
+    }
 
     DPRINT("ScmWaitForLsa() done\n");
 }
@@ -351,23 +346,38 @@
          int nShowCmd)
 {
     HANDLE hScmStartEvent = NULL;
+    HANDLE hScmAutoStartCompleteEvent = NULL;
     SC_RPC_LOCK Lock = NULL;
     BOOL bCanDeleteNamedPipeCriticalSection = FALSE;
     DWORD dwError;
 
     DPRINT("SERVICES: Service Control Manager\n");
 
-    /* Create start event */
-    if (!ScmCreateStartEvent(&hScmStartEvent))
-    {
-        DPRINT1("SERVICES: Failed to create start event\n");
-        goto done;
-    }
-
-    DPRINT("SERVICES: created start event with handle %p.\n", hScmStartEvent);
+    /* We are initializing ourselves */
+    ScmInitialize = TRUE;
+
+    /* Create the start event */
+    if (!ScmCreateControlEvent(&hScmStartEvent,
+                               SCM_START_EVENT,
+                               EVENT_ALL_ACCESS))
+    {
+        DPRINT1("SERVICES: Failed to create the start event\n");
+        goto done;
+    }
+    DPRINT("SERVICES: Created start event with handle %p.\n", hScmStartEvent);
+
+    /* Create the auto-start complete event */
+    if (!ScmCreateControlEvent(&hScmAutoStartCompleteEvent,
+                               SCM_AUTOSTARTCOMPLETE_EVENT,
+                               EVENT_ALL_ACCESS))
+    {
+        DPRINT1("SERVICES: Failed to create the auto-start complete event\n");
+        goto done;
+    }
+    DPRINT("SERVICES: created auto-start complete event with handle %p.\n",
hScmAutoStartCompleteEvent);
 
     /* Create the shutdown event */
-    hScmShutdownEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
+    hScmShutdownEvent = CreateEventW(NULL, TRUE, FALSE, NULL);
     if (hScmShutdownEvent == NULL)
     {
         DPRINT1("SERVICES: Failed to create shutdown event\n");
@@ -385,7 +395,7 @@
     /* Read the control set values */
     if (!ScmGetControlSetValues())
     {
-        DPRINT1("SERVICES: failed to read the control set values\n");
+        DPRINT1("SERVICES: Failed to read the control set values\n");
         goto done;
     }
 
@@ -393,49 +403,58 @@
     dwError = ScmCreateServiceDatabase();
     if (dwError != ERROR_SUCCESS)
     {
-        DPRINT1("SERVICES: failed to create SCM database (Error %lu)\n",
dwError);
-        goto done;
-    }
+        DPRINT1("SERVICES: Failed to create SCM database (Error %lu)\n",
dwError);
+        goto done;
+    }
+
+    /* Wait for the LSA server */
+    ScmWaitForLsa();
 
     /* Update the services database */
     ScmGetBootAndSystemDriverState();
 
-    /* Register the Service Control Manager process with CSRSS */
+    /* Register the Service Control Manager process with the ReactOS Subsystem */
     if (!RegisterServicesProcess(GetCurrentProcessId()))
     {
         DPRINT1("SERVICES: Could not register SCM process\n");
         goto done;
     }
 
-    /* Acquire the service start lock until autostart services have been started */
+    /*
+     * Acquire the user service start lock until
+     * auto-start services have been started.
+     */
     dwError = ScmAcquireServiceStartLock(TRUE, &Lock);
     if (dwError != ERROR_SUCCESS)
     {
-        DPRINT1("SERVICES: failed to acquire the service start lock (Error
%lu)\n", dwError);
+        DPRINT1("SERVICES: Failed to acquire the service start lock (Error
%lu)\n", dwError);
         goto done;
     }
 
     /* Start the RPC server */
     ScmStartRpcServer();
 
-    DPRINT("SERVICES: Initialized.\n");
-
     /* Signal start event */
     SetEvent(hScmStartEvent);
 
+    DPRINT("SERVICES: Initialized.\n");
+
     /* Register event handler (used for system shutdown) */
     SetConsoleCtrlHandler(ShutdownHandlerRoutine, TRUE);
 
-    /* Wait for the LSA server */
-    ScmWaitForLsa();
-
     /* Start auto-start services */
     ScmAutoStartServices();
 
+    /* Signal auto-start complete event */
+    SetEvent(hScmAutoStartCompleteEvent);
+
     /* FIXME: more to do ? */
 
     /* Release the service start lock */
     ScmReleaseServiceStartLock(&Lock);
+
+    /* Initialization finished */
+    ScmInitialize = FALSE;
 
     DPRINT("SERVICES: Running.\n");
 
@@ -451,6 +470,10 @@
     if (hScmShutdownEvent != NULL)
         CloseHandle(hScmShutdownEvent);
 
+    /* Close the auto-start complete event */
+    if (hScmAutoStartCompleteEvent != NULL)
+        CloseHandle(hScmAutoStartCompleteEvent);
+
     /* Close the start event */
     if (hScmStartEvent != NULL)
         CloseHandle(hScmStartEvent);
@@ -458,7 +481,6 @@
     DPRINT("SERVICES: Finished.\n");
 
     ExitThread(0);
-
     return 0;
 }
 

Modified: trunk/reactos/base/system/services/services.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/system/services/servi…
==============================================================================
--- trunk/reactos/base/system/services/services.h [iso-8859-1] (original)
+++ trunk/reactos/base/system/services/services.h [iso-8859-1] Sun Mar 17 12:39:44 2013
@@ -86,6 +86,7 @@
 extern LIST_ENTRY ServiceListHead;
 extern LIST_ENTRY GroupListHead;
 extern LIST_ENTRY ImageListHead;
+extern BOOL ScmInitialize;
 extern BOOL ScmShutdown;