Refactor the process of choosing validators.

src/trusted/service_runtime/sel_ldr.h

Index: src/trusted/service_runtime/sel_ldr.h
diff --git a/src/trusted/service_runtime/sel_ldr.h b/src/trusted/service_runtime/sel_ldr.h
index b2d4a6879cc2fe09d724127d7f229140ad3d341b..6e56c97c38e63033dbc5a877f43479c1f2bc682b 100644
--- a/src/trusted/service_runtime/sel_ldr.h
+++ b/src/trusted/service_runtime/sel_ldr.h
@@ -54,6 +54,7 @@
 #include "native_client/src/trusted/service_runtime/name_service/name_service.h"
 
 #include "native_client/src/trusted/validator/cpufeatures.h"
+#include "native_client/src/trusted/validator/validation_status.h"
 
 EXTERN_C_BEGIN
 
@@ -98,6 +99,43 @@ enum NaClDebugExceptionHandlerState {
 typedef int (*NaClAttachDebugExceptionHandlerFunc)(void *info, size_t size);
 #endif
 
+/* Function type for applying a Validator.
+ * This function is a part of NaCl Validator API: trusted/validator/ncvalidate.h
+ */
+typedef NaClValidationStatus (*NaClValidateFunc) (

[Nick Bray, 2012/04/25 20:57:42]

This is part of the validator interface, not part of sel_ldr.  My instinct is
that these function definitions (and the validation status enums) should be in
src/validator/ncvalidate.h.  The new interface will eventually replace the old,
no reason to add a new file.

[pasko-google - do not use, 2012/04/26 15:17:01]

Done.

+    uintptr_t guest_addr,
+    uint8_t *data,
+    size_t size,
+    int stubout_mode,
+    int readonly_text,
+    const NaClCPUFeatures *cpu_features,
+    struct NaClValidationCache *cache);
+
+/* Function type for applying a Validator to copy code from an existing code
+ * segment to a new code segment.
+ * This function is a part of NaCl Validator API: trusted/validator/ncvalidate.h
+ */
+typedef NaClValidationStatus (*NaClValidateCopyFunc) (
+    uintptr_t guest_addr,
+    uint8_t *data_old,
+    uint8_t *data_new,
+    size_t size,
+    const NaClCPUFeatures *cpu_features);
+
+/* Function type for applying a Validator on small updates to previously
+ * validated code segments.
+ * This function is a part of NaCl Validator API: trusted/validator/ncvalidate.h
+ */
+typedef NaClValidationStatus (*NaClValidateCodeReplacementFunc) (
+    uintptr_t guest_addr,
+    uint8_t *data_old,
+    uint8_t *data_new,
+    size_t size,
+    const NaClCPUFeatures *cpu_features);
+
+/* Make a messy choice for validating functions and record it in NaClApp. */
+void NaClSelectValidator(struct NaClApp *nap);
+
 struct NaClApp {
   /*
    * public, user settable prior to app start.
@@ -355,7 +393,11 @@ struct NaClApp {
   enum NaClDebugExceptionHandlerState debug_exception_handler_state;
   NaClAttachDebugExceptionHandlerFunc attach_debug_exception_handler_func;
 #endif
-  int                       enable_dfa_validator;
+
+  /* Functions for applying validation. */
+  NaClValidateFunc          validate_func;

[Nick Bray, 2012/04/25 20:57:42]

I was envisioning these functions in a separate (but embedded) structure that
also contained the bundle_size parameter.  (bundle_size can be dealt with later,
but it is fundamentally a property of the validation model) This would allow
each validator to declare a InitValidatorInterface(...) function to fill out
this structure.  Less coupling.

[pasko-google - do not use, 2012/04/26 15:17:01]

well, I thought about this same possibility at first, but then on callsites:
nap->validate_func(...) is less clutter than:
nap->validator_interface->validate_func(...)
and then I'm not pleased

Less coupling is good when we have use cases for using components separately or
abstractions that are very natural (and named intuitively). struct
ValidatorInterface does not sound as a positive name for reader's intuition.

Generally I do not mind this decoupling, but for that please give advice on
naming the struct and fields. 

[Nick Bray, 2012/04/27 00:41:30]

Yeah, the deref chain isn't pretty, but I figured it was just the cost of doing
business.  You could always do it in two steps:

validator = nap->validator;
validator->validate(...);

... or even 3, if you wanted to go crazy.
The type could be NaClValidatorInterface, but the field in NaClApp could just be
"validator".

If these functions were put inside a structure, you could also drop the _func
postfix.  validate_copy could also be changed to copy_code - it doesn't
"validate" beyond requiring that it can decode the instructions.

I think there's precedent for function pointers being CamelCase, too.

+  NaClValidateCopyFunc      validate_copy_func;
+  NaClValidateCodeReplacementFunc validate_code_replacement_func;
 };