New policy protobuf protocol.

chrome/tools/build/generate_policy_source.py

Index: chrome/tools/build/generate_policy_source.py
diff --git a/chrome/tools/build/generate_policy_source.py b/chrome/tools/build/generate_policy_source.py
index 1051092ece054d10ab6d4524bdecdef07fd38091..4e313db960744463d4454461e7360ea53497d642 100644
--- a/chrome/tools/build/generate_policy_source.py
+++ b/chrome/tools/build/generate_policy_source.py
@@ -29,6 +29,12 @@ def main():
   parser.add_option("--pth", "--policy-type-header", dest="type_path",
                     help="generate header file for policy type enumeration",
                     metavar="FILE");
+  parser.add_option("--ppb", "--policy-protobuf", dest="proto_path",
+                    help="generate cloud policy protobuf file",
+                    metavar="FILE");
+  parser.add_option("--ppd", "--protobuf-decoder", dest="decoder_path",
+                    help="generate CPP code decoding the policy protobuf",

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

In my experience CPP most of the time refers to the C PreProcessor and most
people use CXX to refer to C++. But why not just say C++? ;)

[Jakob Kummerow, 2011/02/03 14:36:52]

Done.

+                    metavar="FILE");
 
   (opts, args) = parser.parse_args();
 
@@ -38,27 +44,45 @@ def main():
     sys.exit(2)
   template_file_contents = _LoadJSONFile(args[1]);
   if opts.header_path is not None:
-    _WritePolicyConstantHeader(template_file_contents,
-                               args,
-                               opts);
+    _WritePolicyConstantHeader(template_file_contents, args, opts);
   if opts.source_path is not None:
-    _WritePolicyConstantSource(template_file_contents,
-                               args,
-                               opts);
+    _WritePolicyConstantSource(template_file_contents, args, opts);
   if opts.type_path is not None:
-    _WritePolicyTypeEnumerationHeader(template_file_contents,
-                                      args,
-                                      opts);
+    _WritePolicyTypeEnumerationHeader(template_file_contents, args, opts);
+  if opts.proto_path is not None:
+    _WriteProtobuf(template_file_contents, args, opts.proto_path)
+  if opts.decoder_path is not None:
+    _WriteProtobufParser(template_file_contents, args, opts.decoder_path)
 
 
+#------------------ shared helpers ---------------------------------#

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

Is that commonly used comment style in chromium python code?

[Jakob Kummerow, 2011/02/03 14:36:52]

No, I guess it's not. I just felt that this file would benefit from having clear
visual separations of its sections.

 def _OutputGeneratedWarningForC(f, template_file_path):
     f.write('//\n'
             '// DO NOT MODIFY THIS FILE DIRECTLY!\n'
             '// IT IS GENERATED BY generate_policy_source.py\n'
-            '// FROM ' + template_file_path + ' \n'
+            '// FROM ' + template_file_path + '\n'
             '//\n\n')
 
 
+def _GetPolicyNameList(template_file_contents):
+  policy_names = [];
+  for policy in template_file_contents['policy_definitions']:
+    if policy['type'] == 'group':
+      for sub_policy in policy['policies']:
+        policy_names.append(sub_policy['name'])
+    else:
+      policy_names.append(policy['name'])
+  policy_names.sort()
+  return policy_names
+
+
+def _LoadJSONFile(json_file):
+  with open(json_file, "r") as f:
+    text = f.read()
+  return eval(text)
+
+
+#------------------ policy constants header ------------------------#
 def _WritePolicyConstantHeader(template_file_contents, args, opts):
   platform = args[0];
   with open(opts.header_path, "w") as f:
@@ -80,6 +104,7 @@ def _WritePolicyConstantHeader(template_file_contents, args, opts):
             '#endif  // CHROME_COMMON_POLICY_CONSTANTS_H_\n')
 
 
+#------------------ policy constants source ------------------------#
 def _WritePolicyConstantSource(template_file_contents, args, opts):
   platform = args[0];
   with open(opts.source_path, "w") as f:
@@ -103,6 +128,7 @@ def _WritePolicyConstantSource(template_file_contents, args, opts):
             '}  // namespace policy\n')
 
 
+#------------------ policy type enumeration header -----------------#
 def _WritePolicyTypeEnumerationHeader(template_file_contents, args, opts):
   with open(opts.type_path, "w") as f:
     _OutputGeneratedWarningForC(f, args[1])
@@ -120,23 +146,182 @@ def _WritePolicyTypeEnumerationHeader(template_file_contents, args, opts):
             '#endif  // CHROME_BROWSER_POLICY_CONFIGURATION_POLICY_TYPE_H_\n')
 
 
-def _GetPolicyNameList(template_file_contents):
-  policy_names = [];
-  for policy in template_file_contents['policy_definitions']:
-    if policy['type'] == 'group':
-      for sub_policy in policy['policies']:
-        policy_names.append(sub_policy['name'])
-    else:
-      policy_names.append(policy['name'])
-  policy_names.sort()
-  return policy_names
+#------------------ policy protobuf --------------------------------#
+PROTO_HEAD = '''
+syntax = "proto2";
 
+option optimize_for = LITE_RUNTIME;
 
-def _LoadJSONFile(json_file):
-  with open(json_file, "r") as f:
-    text = f.read()
-  return eval(text)
+package enterprise_management;
+
+// Only included in serialized form in the PBs below.

[gfeher, 2011/02/02 08:42:45]

Please update comment.

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

and can't we just switch to repeated string instead of wrapping this in
StringList?

[Jakob Kummerow, 2011/02/03 14:36:52]

Done.

[Jakob Kummerow, 2011/02/03 14:36:52]

As discussed offline: Yes, we could, but we would lose the ability to
differentiate between empty lists and non-existing lists, so we don't.

+message StringList {
+  repeated string entries = 1;
+}
+
+message PolicyOptions {
+  enum PolicyMode {
+    // The user may choose to override the given settings.
+    RECOMMENDED = 1;
+    // The given settings are applied regardless of user choice.
+    MANDATORY = 2;
+  }
+  optional PolicyMode mode = 1;
+}
+

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

Can we have this in a separate .proto that includes the generated stuff?

[Jakob Kummerow, 2011/02/03 14:36:52]

As discussed offline: Yes, we could, but since it's not much code, it's unclear
whether that would be better, so we're leaving it in here for now. If we ever
need more static code here, we can always factor it out into its own file.

+'''
+
+
+def _PolicyTypeToProtobufType(type):
+  if type == 'main':
+    return 'bool'
+  elif type in ('string', 'string-enum'):
+    return 'string'
+  elif type in ('int', 'int-enum'):
+    return 'int64'
+  elif type == 'list':
+    return 'StringList'
+  else:
+    raise NotImplementedError()
+
+
+def _WriteProtobuf(data, args, outfilepath):

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

What's data? Can we have a better name?

[Jakob Kummerow, 2011/02/03 14:36:52]

Done.

+  with open(outfilepath, 'w') as f:
+    _OutputGeneratedWarningForC(f, args[1])
+    f.write(PROTO_HEAD)
+
+    # Write protobufs for individual settings/groups.
+    f.write('// PBs for individual settings.\n\n')
+    for policy in data['policy_definitions']:
+      f.write('message %sProto {\n' % policy['name'])
+      f.write('  optional PolicyOptions policy_options = 1;\n')
+      if policy['type'] == 'group':
+        for sub_policy in policy['policies']:
+          f.write('  optional %s %s = %s;\n' %
+                  (_PolicyTypeToProtobufType(sub_policy['type']),
+                   sub_policy['name'], (sub_policy['protobuf_id'] + 1)))

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

hm, we might want to name the id just ID and not protobuf_id, because of the +1
(and the json file shouldn't know it's being used for proto generation).

[Jakob Kummerow, 2011/02/03 14:36:52]

Done.

+      else:
+        f.write('  optional %s %s = 2;\n' %
+                (_PolicyTypeToProtobufType(policy['type']), policy['name']))

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

Seems like you could use a _WritePolicy function and call it from the two
branches.

[Jakob Kummerow, 2011/02/03 14:36:52]

Done.

+      f.write('}\n\n')
+    # end of: for policy in data['policy_definitions'].
+
+    f.write('// --------------------------------------------------\n'
+            '// Big wrapper PB containing the above groups.\n\n'
+            'message CloudPolicySettings {\n')
+    for policy in data['policy_definitions']:
+      f.write('  optional %sProto %s = %s;\n' %
+              (policy['name'], policy['name'], policy['protobuf_id']))
+    f.write('}\n\n')
+
+
+#------------------ protobuf decoder -------------------------------#
+CPP_HEAD = '''
+#include <limits>
+#include <map>
+#include <string>
+
+#include "base/logging.h"
+#include "base/values.h"
+#include "chrome/browser/policy/proto/device_management_backend.pb.h"
+#include "policy/configuration_policy_type.h"
+
+using google::protobuf::RepeatedPtrField;
+
+namespace policy {
+
+namespace em = enterprise_management;
+
+typedef std::map<ConfigurationPolicyType, Value*> PolicyMapType;
+
+Value* DecodeIntegerValue(google::protobuf::int64 value) {
+  if (value < std::numeric_limits<int>::min() ||
+      value > std::numeric_limits<int>::max()) {
+    LOG(WARNING) << "Integer value " << value
+                 << " out of numeric limits, ignoring.";
+    return NULL;
+  }
+
+  return Value::CreateIntegerValue(static_cast<int>(value));
+}
+
+ListValue* DecodeStringList(const em::StringList& string_list) {
+  ListValue* list_value = new ListValue;
+  RepeatedPtrField<std::string>::const_iterator entry;
+  for (entry = string_list.entries().begin();
+       entry != string_list.entries().end(); ++entry) {
+    list_value->Append(Value::CreateStringValue(*entry));
+  }
+  return list_value;
+}
+
+void DecodePolicy(const em::CloudPolicySettings& policy,
+                  PolicyMapType* mandatory, PolicyMapType* recommended) {
+  DCHECK(mandatory);
+  DCHECK(recommended);
+'''

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

I wouldn't worry to much about this boilerplate header because you need to
generate the proper #includes anyway. Another approach I can see would be to
generate only the DecodePolicy function definition and #include that in some .cc
file that contains the boilerplate.

[Jakob Kummerow, 2011/02/03 14:36:52]

Same as above: as discussed offline, we'll leave this as is for now.

+
+
+CPP_FOOT = '''}
+
+}  // namespace policy
+'''
+
+
+def _CreateValue(type):
+  if type == 'main':
+    return "Value::CreateBooleanValue"
+  elif type in ('int', 'int-enum'):
+    return "DecodeIntegerValue"
+  elif type in ('string', 'string-enum'):
+    return "Value::CreateStringValue"
+  elif type == 'list':
+    return "DecodeStringList"
+  else:
+    raise NotImplementedError()
+
+
+def _GeneratePolicyCode(policy, protoname=None):
+  membername = policy['name'].lower()
+  if protoname is None:
+    protoname = "%s_proto" % policy['name'].lower()

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

can use membername instead of the repeated policy['name'].lower()

[Jakob Kummerow, 2011/02/03 14:36:52]

Done.

+  code = '    if (%s.has_%s()) {\n' % (protoname, membername)
+  code += ('      Value* value = %s(%s.%s());\n' %
+           (_CreateValue(policy['type']), protoname, membername))
+  code += ('      PolicyMapType* result = mandatory;\n'

[gfeher, 2011/02/02 08:42:45]

Nit: rename |result| to |destination|?

[Jakob Kummerow, 2011/02/03 14:36:52]

Done.

+           '      if (%s.has_policy_options() &&\n'
+           '          (%s.policy_options().mode() == '
+                      'em::PolicyOptions::RECOMMENDED)) {\n' %

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

I think I would rather have used a switch(%s.policy_options().mode()) here,
which is future-proof in a way that the compiler will notice if we ever add a
third enum value. WDYT?

[Jakob Kummerow, 2011/02/03 14:36:52]

Done.

+           (protoname, protoname))
+  code += ('        result = recommended;\n'
+           '      }\n'
+           '      result->insert(std::make_pair(kPolicy%s, value));\n' %
+           policy['name'])
+  code += '    }\n'
+  return code
+
+
+def _WriteProtobufParser(data, args, outfilepath):
+  with open(outfilepath, 'w') as f:
+    _OutputGeneratedWarningForC(f, args[1])
+    f.write(CPP_HEAD)
+    for policy in data['policy_definitions']:
+      policyname = policy['name']
+      membername = policyname.lower()
+      proto_type = "%sProto" % policyname
+      protoname = "%s_proto" % policyname.lower()

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

why not use membername?

[Jakob Kummerow, 2011/02/03 14:36:52]

Done.

+      f.write('  if (policy.has_%s()) {\n' % membername)
+      f.write('    const em::%s& %s = policy.%s();\n' %
+              (proto_type, protoname, membername))
+      if policy['type'] == 'group':
+        for sub_policy in policy['policies']:
+          f.write(_GeneratePolicyCode(sub_policy, protoname))

[Mattias Nissler (ping if slow), 2011/02/02 12:27:55]

Why not just pass f to the function instead of building a string and returning
it?

[Jakob Kummerow, 2011/02/03 14:36:52]

Done.

+      else:
+        f.write(_GeneratePolicyCode(policy))
+      f.write('  }\n')
+    f.write(CPP_FOOT)
 
 
+#------------------ main() -----------------------------------------#
 if __name__ == '__main__':
   main();