NaCl: Expose NaClApp to embedding layer.

src/trusted/service_runtime/sel_main_chrome.c

 /*
  * Copyright (c) 2012 The Native Client Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "native_client/src/trusted/service_runtime/sel_main_chrome.h"
 
 #include "native_client/src/include/portability.h"
 #include "native_client/src/include/portability_io.h"
@@ skipping 106 matching lines @@
   if (errcode != LOAD_OK) {
     NaClLog(LOG_FATAL,
             "NaClLoadIrt: Failed to load the integrated runtime (IRT): %s\n",
             NaClErrorString(errcode));
   }
 
   NaClMetadataDtor(&metadata);
   NaClDescUnref(nd);
 }
 
-void NaClChromeMainStart(struct NaClChromeMainArgs *args) {
+struct NaClApp* NaClAppCreate(void) {

[Mark Seaborn, 2014/01/17 16:42:06]

Spacing style should be " *" in NaCl-side code (same elsewhere in this change).

+  struct NaClApp* nap = (struct NaClApp*)malloc(sizeof(struct NaClApp));
+  if (!nap)

[Mark Seaborn, 2014/01/17 16:42:06]

NaCl style is "nap == NULL"

+    return NULL;
+
+  memset(nap, 0, sizeof(struct NaClApp));
+  if (NaClAppCtor(nap) != 0) {

[Mark Seaborn, 2014/01/17 16:42:06]

There's a correctness problem here.  If NaClAppCreate() is called before
NaClChromeMainStartApp(), the former will happen without NaClAllModulesInit()
having been called.

Can you run trybots for NaCl changes before sending them for review, please?  I
don't know if this works with sel_main_chrome_test on all platforms.

BTW, "git cl try" only works for changes to the Chromium repo, not any other
repo.  You have to use "git try" instead.

+    free(nap);
+    return NULL;
+  }
+  return nap;
+}
+
+void NaClChromeMainStartApp(struct NaClApp *nap,
+                            struct NaClChromeMainArgs *args) {
   char *av[1];
   int ac = 1;
   const char **envp;
-  struct NaClApp state;
-  struct NaClApp *nap = &state;
   NaClErrorCode errcode = LOAD_INTERNAL;
   int ret_code = 1;
   struct NaClEnvCleanser env_cleanser;
   int skip_qualification;
 
 #if NACL_OSX
   /* Mac dynamic libraries cannot access the environ variable directly. */
   envp = (const char **) *_NSGetEnviron();
 #else
   /* Overzealous code style check is overzealous. */
   /* @IGNORE_LINES_FOR_CODE_HYGIENE[1] */
   extern char **environ;
   envp = (const char **) environ;
 #endif
 
 #if NACL_LINUX || NACL_OSX
   /* This needs to happen before NaClAllModulesInit(). */
   if (args->urandom_fd != -1)
     NaClSecureRngModuleSetUrandomFd(args->urandom_fd);
 #endif
 
   /*
    * Clear state so that NaClBootstrapChannelErrorReporter will be
    * able to know if the bootstrap channel is available or not.
    */
-  memset(&state, 0, sizeof state);
   NaClAllModulesInit();
   NaClBootstrapChannelErrorReporterInit();
-  NaClErrorLogHookInit(NaClBootstrapChannelErrorReporter, &state);
+  NaClErrorLogHookInit(NaClBootstrapChannelErrorReporter, nap);
 
   /* to be passed to NaClMain, eventually... */
   av[0] = "NaClMain";
 
-  if (NACL_FI_ERROR_COND("AppCtor", !NaClAppCtor(&state))) {
-    NaClLog(LOG_FATAL, "Error while constructing app state\n");
-    goto done;
-  }
-
   errcode = LOAD_OK;
 
   /* Allow or disallow dyncode API based on args. */
   nap->enable_dyncode_syscalls = args->enable_dyncode_syscalls;
   nap->initial_nexe_max_code_bytes = args->initial_nexe_max_code_bytes;
 
 #if NACL_LINUX
   g_prereserved_sandbox_size = args->prereserved_sandbox_size;
 #endif
 #if NACL_LINUX || NACL_OSX
@@ skipping 81 matching lines @@
         args->attach_debug_exception_handler_func;
 #else
 # error Unknown host OS
 #endif
   }
 #if NACL_LINUX
   NaClSignalHandlerInit();
 #endif
 
   /* Give debuggers a well known point at which xlate_base is known.  */
-  NaClGdbHook(&state);
+  NaClGdbHook(nap);
 
   NaClCreateServiceSocket(nap);
   /*
    * LOG_FATAL errors that occur before NaClSetUpBootstrapChannel will
    * not be reported via the crash log mechanism (for Chromium
    * embedding of NaCl, shown in the JavaScript console).
    *
    * Some errors, such as due to NaClRunSelQualificationTests, do not
    * trigger a LOG_FATAL but instead set module_load_status to be sent
    * in the start_module RPC reply.  Log messages associated with such
@@ skipping 116 matching lines @@
    * Instead, we wait for the hard-shutdown on the command channel.
    */
   if (LOAD_OK != errcode) {
     NaClBlockIfCommandChannelExists(nap);
   }
 
   NaClAllModulesFini();
 
   NaClExit(ret_code);
 }
+
+void NaClChromeMainStart(struct NaClChromeMainArgs *args) {
+  struct NaClApp *nap = NaClAppCreate();

[dmichael (off chromium), 2014/01/16 21:30:26]

Is there a need to do it on the heap here? Could you just do it on the stack in
this case, and get rid of NaClAppCreate?

[dmichael (off chromium), 2014/01/17 16:00:46]

Okay, we just chatted. Since:
1) This code is going to be deleted soon anyway
and
2) Using NaClAppCreate here exercises that function (whereas a stack variable
wouldn't)
...
the code's good as you have it. I think a TODO to delete this function wouldn't
hurt.

+  if (!nap) {
+    NaClLog(LOG_FATAL, "NaClChromeMainStart: Could not create NaClApp.\n");
+  }
+  NaClChromeMainStartApp(nap, args);
+}