Support replacement of IP address resolutions via command line flag

net/base/ip_mapping_rules.h

+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_BASE_IP_MAPPING_RULES_H_
+#define NET_BASE_IP_MAPPING_RULES_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "net/base/address_list.h"

[wtc, 2014/02/11 23:52:30]

1. Nit: I think you can include "base/macros.h" instead of "base/basictypes.h".
I think the only thing you need from the header is DISALLOW_COPY_AND_ASSIGN.

2. Nit: You should be able to use a forward declaration of class AddressList
instead of including "net/base/address_list.h".

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+#include "net/base/net_export.h"
+
+namespace net {
+
+class NET_EXPORT_PRIVATE IPMappingRules {

[wtc, 2014/02/11 23:52:30]

Nit: it would be nice to have a short comment describing what this class is.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+ public:
+  IPMappingRules();
+  ~IPMappingRules();
+
+  // Modifies |*addresses| based on the current rules. Returns true if
+  // |addresses| was modified, false otherwise.
+  bool RewriteAddresses(AddressList* addresses) const;
+
+  // Adds a rule to this mapper.
+  // Rules are evaluated against an address list until one is found that

[wtc, 2014/02/11 23:52:30]

Nit: "one" is ambiguous because it could mean a rule or an address. I suggest
saying "a rule".

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  // applies, causing a modification of the address resolution list.

[wtc, 2014/02/11 23:52:30]

Nit: remove "resolution".

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  // Each rule consists of one or more of the following pattern and action
+  // directives.
+  //
+  //   RULE : CHANGE_DIRECTIVE | EMPTY

[wtc, 2014/02/11 23:52:30]

Nit: remove the space before ":".

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  //   CHANGE_DIRECTIVE: "PREFACE" SPACE IP_PATTERN SPACE IP_LITERAL

[wtc, 2014/02/11 23:52:30]

You should document the PREFACE directive somewhere.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  //   SPACE: " "
+  //   EMPTY:
+  //
+  //   IP_LITERAL: IP_V6_LITERAL | IP_V4_LITERAL
+  //
+  //   IP_V4_LITERAL: OCTET "." OCTET "." OCTET "." OCTET
+  //   OCTET: decimal number in range 0 ... 255
+  //
+  //   IP_V6_LITERAL:  HEX_COMPONENT  | IP_V6_LITERAL ":" HEX_COMPONENT

[wtc, 2014/02/11 23:52:30]

This may not be a complete specification of IPv6 literals. For eample, "::" and
"::1" are both IPv6 literals.

Update: I see that you require an IP_V6_LITERAL to have 8 components. In that
case, I think this IP_V6_LITERAL is still not correct because it allows a single
HEX_COMPONENT.

[jar (doing other things), 2014/02/15 21:14:56]

I made this explicit in a new comment below.

+  //   HEX_COMPONENT: hexidecimal values with no more than 4 hex digits

[wtc, 2014/02/11 23:52:30]

Typo: hexidecimal => hexadecimal

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  //
+  //   IP_PATTERN: IP_V6_PATTERN | IP_V4_PATTERN
+  //
+  //   IP_V6_PATTERN: HEX_PATTERN | IP_V6_PATTERN ":" HEX_PATTERN
+  //   HEX_PATTERN:  HEX_COMPONENT | "[" HEX_GROUP_PATTERN "]" | "[*]"

[wtc, 2014/02/12 00:11:15]

Please fix the grammar here and on line 53. '*' should not be inside [].

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  //   HEX_GROUP_PATTERN: HEX_RANGE | HEX_GROUP_PATTERN "," HEX_RANGE
+  //   HEX_RANGE: HEX_COMPONENT |  HEX_COMPONENT "-" HEX_COMPONENT
+  //
+  //   IP_V4_PATTERN: OCTET_PATTERN "." OCTET_PATTERN "." OCTET_PATTERN
+  //                  "." OCTET_PATTERN
+  //   OCTET_PATTERN:  OCTET | "[" OCTET_GROUP_PATTERN "]" | "[*]"
+  //   OCTET_GROUP_PATTERN: OCTET_ITEM | OCTET_GROUP_PATTERN "," OCTET_ITEM
+  //   OCTET_ITEM: OCTET | OCTET "-" OCTET

[wtc, 2014/02/11 23:52:30]

I think OCTET_ITEM should be renamed OCTET_RANGE to be consistent with
HEX_RANGE.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  //
+  //  All literals are required to have all their components specified (4
+  //  components for IPv4, and 8 for IPv6).

[wtc, 2014/02/11 23:52:30]

Nit: these two lines probably should have only one space in the front.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  // Returns true if the rule was successfully parsed and added.
+  bool AddRuleFromString(const std::string& rule_string);
+
+  // Sets the rules from a semicolon separated list of rules.
+  // Returns true if all the rules were successfully parsed and added.
+  bool SetRulesFromString(const std::string& rules_string);
+
+ private:
+  // Delete all rules, starting with first_rule_.
+  void DeleteRules();
+
+  class MapRule;

[wtc, 2014/02/11 23:52:30]

Nit: our Style Guide seems to recommend placing the forward declarations of
classes at the beginning of the "private" section.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  // Singly linked list of rules.  We own (and destroy) all rules by walking
+  // the list.
+  MapRule* first_rule_;

[Ryan Hamilton, 2014/02/12 00:44:03]

nit: instead of implementing a linked-list manually, I recommend using an
std::list (or std::vector) of MapRule*s.  You'll need to add
STLDeleteElements(&rules_) to your destructor, but since you already had to add
(and implement) DeleteRules, it will actually end up less code.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(IPMappingRules);
+};
+
+}  // namespace net
+
+#endif  // NET_BASE_IP_MAPPING_RULES_H_