)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7183,"name":"Xu Han Peng","email":"xuhanp@linux.vnet.ibm.com","username":"xuhanp"},"change_message_id":"d1a165f76c1ba6482d10ac64815c2b479755adc9","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Support Router Advertisement Daemon (radvd) for IPv6"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Launch radvd from inside l3 agent when any router port has an IPv6 address. If"},{"line_number":10,"context_line":"ipv6-ra-mode is slaac, advertise the prefix associated with the port;"},{"line_number":11,"context_line":"otherwise, advertise default route only."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Change-Id: Ib8b0b3e71f7af9afa769c41357c66f88f4326807"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"baada198_6fde3513","line":10,"updated":"2014-07-02 03:23:43.000000000","message":"Should also include the case ipv6_ra_mode is dhcpv6_stateless?","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"}],"etc/neutron/rootwrap.d/l3.filters":[{"author":{"_account_id":4656,"name":"Sean M. Collins","email":"sean@coreitpro.com","username":"scollins"},"change_message_id":"57bbecd44db57f1e52777090177c9bc4104b0f4c","unresolved":false,"context_lines":[{"line_number":15,"context_line":"sysctl: CommandFilter, sysctl, root"},{"line_number":16,"context_line":"route: CommandFilter, route, root"},{"line_number":17,"context_line":"radvd: CommandFilter, radvd, root"},{"line_number":18,"context_line":"cat: CommandFilter, cat, root"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"# metadata proxy"},{"line_number":21,"context_line":"metadata_proxy: CommandFilter, neutron-ns-metadata-proxy, root"}],"source_content_type":"application/octet-stream","patch_set":2,"id":"1ae5cdf2_690975dc","line":18,"updated":"2014-06-26 18:43:55.000000000","message":"This also could be a security problem. I don\u0027t think I want Neutron being able to read any file on the filesystem as root.","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"84270082f03f163b576db677561ddfd6b9fbffdd","unresolved":false,"context_lines":[{"line_number":15,"context_line":"sysctl: CommandFilter, sysctl, root"},{"line_number":16,"context_line":"route: CommandFilter, route, root"},{"line_number":17,"context_line":"radvd: CommandFilter, radvd, root"},{"line_number":18,"context_line":"cat: CommandFilter, cat, root"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"# metadata proxy"},{"line_number":21,"context_line":"metadata_proxy: CommandFilter, neutron-ns-metadata-proxy, root"}],"source_content_type":"application/octet-stream","patch_set":2,"id":"1ae5cdf2_aed8de7f","line":18,"in_reply_to":"1ae5cdf2_690975dc","updated":"2014-06-26 20:49:51.000000000","message":"I think that I don\u0027t need root and a namespace for \u0027cat\u0027. I will remove it","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"}],"neutron/agent/l3_agent.py":[{"author":{"_account_id":3217,"name":"Ian Wells","username":"ijw-ubuntu"},"change_message_id":"f76b2e23ddd3d456e3d2bd33cb8682506a6642de","unresolved":false,"context_lines":[{"line_number":426,"context_line":""},{"line_number":427,"context_line":"    def _get_conf_file_name(self, ri, kind, ensure_conf_dir\u003dFalse):"},{"line_number":428,"context_line":"        \"\"\"Returns the file name for a given kind of config file.\"\"\""},{"line_number":429,"context_line":"        confs_dir \u003d os.path.abspath(os.path.normpath(self.conf.ra_confs))"},{"line_number":430,"context_line":"        conf_dir \u003d os.path.join(confs_dir, ri.router_id)"},{"line_number":431,"context_line":"        if ensure_conf_dir:"},{"line_number":432,"context_line":"            if not os.path.isdir(conf_dir):"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_64351cf6","line":429,"updated":"2014-06-26 19:43:05.000000000","message":"Returns *RA* conf file names, but I don\u0027t think this is in an RA-specific class and the name of the function doesn\u0027t hint at the RA relationship.  Is the name specific enough?  (That or you make your config files generic, in which case you shouldn\u0027t call your conf var \u0027ra_confs\u0027 but merely \u0027router_confs\u0027 - which might be better, actually.)","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"84270082f03f163b576db677561ddfd6b9fbffdd","unresolved":false,"context_lines":[{"line_number":426,"context_line":""},{"line_number":427,"context_line":"    def _get_conf_file_name(self, ri, kind, ensure_conf_dir\u003dFalse):"},{"line_number":428,"context_line":"        \"\"\"Returns the file name for a given kind of config file.\"\"\""},{"line_number":429,"context_line":"        confs_dir \u003d os.path.abspath(os.path.normpath(self.conf.ra_confs))"},{"line_number":430,"context_line":"        conf_dir \u003d os.path.join(confs_dir, ri.router_id)"},{"line_number":431,"context_line":"        if ensure_conf_dir:"},{"line_number":432,"context_line":"            if not os.path.isdir(conf_dir):"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_3b35eef6","line":429,"in_reply_to":"1ae5cdf2_64351cf6","updated":"2014-06-26 20:49:51.000000000","message":"The idea of this routine  and the next one is actually borrowed from dhcp.py. are you ok with them being called _get_ra_conf_file_name and _get_value_from_ra_conf_file","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":3217,"name":"Ian Wells","username":"ijw-ubuntu"},"change_message_id":"f76b2e23ddd3d456e3d2bd33cb8682506a6642de","unresolved":false,"context_lines":[{"line_number":434,"context_line":""},{"line_number":435,"context_line":"        return os.path.join(conf_dir, kind)"},{"line_number":436,"context_line":""},{"line_number":437,"context_line":"    def _get_value_from_conf_file(self, ri, kind, converter\u003dNone):"},{"line_number":438,"context_line":"        \"\"\"A helper function to read a value from one of the state files.\"\"\""},{"line_number":439,"context_line":"        file_name \u003d self._get_conf_file_name(ri, kind)"},{"line_number":440,"context_line":"        msg \u003d _(\u0027Error while reading %s\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_f8b54c19","line":437,"updated":"2014-06-26 19:43:05.000000000","message":"This is a bit weird, particularly because you\u0027re reading a file using this function which constructs the file name, then later constructing the filename again because the previous version wasn\u0027t constructed in context.\n\nI would rename this _get_value_from_file and pass in the conf file name.","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"84270082f03f163b576db677561ddfd6b9fbffdd","unresolved":false,"context_lines":[{"line_number":434,"context_line":""},{"line_number":435,"context_line":"        return os.path.join(conf_dir, kind)"},{"line_number":436,"context_line":""},{"line_number":437,"context_line":"    def _get_value_from_conf_file(self, ri, kind, converter\u003dNone):"},{"line_number":438,"context_line":"        \"\"\"A helper function to read a value from one of the state files.\"\"\""},{"line_number":439,"context_line":"        file_name \u003d self._get_conf_file_name(ri, kind)"},{"line_number":440,"context_line":"        msg \u003d _(\u0027Error while reading %s\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_7b3bf6e9","line":437,"in_reply_to":"1ae5cdf2_f8b54c19","updated":"2014-06-26 20:49:51.000000000","message":"see above","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":3217,"name":"Ian Wells","username":"ijw-ubuntu"},"change_message_id":"f76b2e23ddd3d456e3d2bd33cb8682506a6642de","unresolved":false,"context_lines":[{"line_number":455,"context_line":"        ports \u003d ri.router.get(l3_constants.INTERFACE_KEY, [])"},{"line_number":456,"context_line":"        has_ipv6 \u003d False"},{"line_number":457,"context_line":"        for p in ports:"},{"line_number":458,"context_line":"            if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":459,"context_line":"                has_ipv6 \u003d True"},{"line_number":460,"context_line":"                break"},{"line_number":461,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_e4720ce2","line":458,"updated":"2014-06-26 19:43:05.000000000","message":"This is my ignorance, rather than a criticism: what about ports that have v4 and v6 addresses both?  This code seems to assume that a port has precisely one subnet, and precisely one address which is either v4 or v6, but that doesn\u0027t seem logical.","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"84270082f03f163b576db677561ddfd6b9fbffdd","unresolved":false,"context_lines":[{"line_number":455,"context_line":"        ports \u003d ri.router.get(l3_constants.INTERFACE_KEY, [])"},{"line_number":456,"context_line":"        has_ipv6 \u003d False"},{"line_number":457,"context_line":"        for p in ports:"},{"line_number":458,"context_line":"            if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":459,"context_line":"                has_ipv6 \u003d True"},{"line_number":460,"context_line":"                break"},{"line_number":461,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_0efe3205","line":458,"in_reply_to":"1ae5cdf2_e4720ce2","updated":"2014-06-26 20:49:51.000000000","message":"This is not a concern for now. Each router port allows one address only for now.","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"6493cac4a91594b43bd455dff0e2f657a96eeca0","unresolved":false,"context_lines":[{"line_number":462,"context_line":"        if not has_ipv6:"},{"line_number":463,"context_line":"            return"},{"line_number":464,"context_line":""},{"line_number":465,"context_line":"        buf \u003d six.StringIO()"},{"line_number":466,"context_line":"        ra_cfg_file \u003d self._get_conf_file_name(ri, \u0027radvd.conf\u0027, True)"},{"line_number":467,"context_line":"        pid \u003d self._get_value_from_conf_file(ri, \u0027pid\u0027, int)"},{"line_number":468,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"dab895d6_8541e50d","line":465,"updated":"2014-06-29 19:16:15.000000000","message":"I know you have copied this from dhcp.py, but I can\u0027t see any reason to use StringIO instead of plain strings here (or in dhcp).","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":4656,"name":"Sean M. Collins","email":"sean@coreitpro.com","username":"scollins"},"change_message_id":"55642e916e41b37e1e285370c73f6036359e0c9e","unresolved":false,"context_lines":[{"line_number":469,"context_line":"        for p in ports:"},{"line_number":470,"context_line":"            if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":471,"context_line":"                interface_name \u003d self.get_internal_device_name(p[\u0027id\u0027])"},{"line_number":472,"context_line":"                if p[\u0027subnet\u0027][\u0027ipv6_ra_mode\u0027] \u003d\u003d \u0027slaac\u0027:"},{"line_number":473,"context_line":"                    conf_str \u003d \"\"\"interface %s"},{"line_number":474,"context_line":"{"},{"line_number":475,"context_line":"   AdvSendAdvert on;"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_2f04d5c4","line":472,"updated":"2014-06-26 15:50:10.000000000","message":"This should be using the constant constants.IPV6_SLAAC instead of a string \u0027slaac\u0027.","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"84270082f03f163b576db677561ddfd6b9fbffdd","unresolved":false,"context_lines":[{"line_number":469,"context_line":"        for p in ports:"},{"line_number":470,"context_line":"            if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":471,"context_line":"                interface_name \u003d self.get_internal_device_name(p[\u0027id\u0027])"},{"line_number":472,"context_line":"                if p[\u0027subnet\u0027][\u0027ipv6_ra_mode\u0027] \u003d\u003d \u0027slaac\u0027:"},{"line_number":473,"context_line":"                    conf_str \u003d \"\"\"interface %s"},{"line_number":474,"context_line":"{"},{"line_number":475,"context_line":"   AdvSendAdvert on;"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_4e04ba13","line":472,"in_reply_to":"1ae5cdf2_2f04d5c4","updated":"2014-06-26 20:49:51.000000000","message":"sure thing","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":7183,"name":"Xu Han Peng","email":"xuhanp@linux.vnet.ibm.com","username":"xuhanp"},"change_message_id":"d1a165f76c1ba6482d10ac64815c2b479755adc9","unresolved":false,"context_lines":[{"line_number":469,"context_line":"        for p in ports:"},{"line_number":470,"context_line":"            if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":471,"context_line":"                interface_name \u003d self.get_internal_device_name(p[\u0027id\u0027])"},{"line_number":472,"context_line":"                if p[\u0027subnet\u0027][\u0027ipv6_ra_mode\u0027] \u003d\u003d \u0027slaac\u0027:"},{"line_number":473,"context_line":"                    conf_str \u003d \"\"\"interface %s"},{"line_number":474,"context_line":"{"},{"line_number":475,"context_line":"   AdvSendAdvert on;"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_0fc69949","line":472,"in_reply_to":"1ae5cdf2_4e04ba13","updated":"2014-07-02 03:23:43.000000000","message":"I think we should also consider ipv6_ra_mode\u003ddhcpv6_stateless where VM also obtains RA from RADVD?","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"fd840ab9781a275f958679e96c6c382aca1f731e","unresolved":false,"context_lines":[{"line_number":469,"context_line":"        for p in ports:"},{"line_number":470,"context_line":"            if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":471,"context_line":"                interface_name \u003d self.get_internal_device_name(p[\u0027id\u0027])"},{"line_number":472,"context_line":"                if p[\u0027subnet\u0027][\u0027ipv6_ra_mode\u0027] \u003d\u003d \u0027slaac\u0027:"},{"line_number":473,"context_line":"                    conf_str \u003d \"\"\"interface %s"},{"line_number":474,"context_line":"{"},{"line_number":475,"context_line":"   AdvSendAdvert on;"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_df46f34e","line":472,"in_reply_to":"baada198_0fc69949","updated":"2014-07-10 15:02:56.000000000","message":"I think for the time being, I will just follow _check_if_subnet_uses_eui64() in db_base_plugin_v2.py to determine that.","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"6493cac4a91594b43bd455dff0e2f657a96eeca0","unresolved":false,"context_lines":[{"line_number":470,"context_line":"            if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":471,"context_line":"                interface_name \u003d self.get_internal_device_name(p[\u0027id\u0027])"},{"line_number":472,"context_line":"                if p[\u0027subnet\u0027][\u0027ipv6_ra_mode\u0027] \u003d\u003d \u0027slaac\u0027:"},{"line_number":473,"context_line":"                    conf_str \u003d \"\"\"interface %s"},{"line_number":474,"context_line":"{"},{"line_number":475,"context_line":"   AdvSendAdvert on;"},{"line_number":476,"context_line":"   MinRtrAdvInterval 3;"}],"source_content_type":"text/x-python","patch_set":2,"id":"dab895d6_c5f83dc7","line":473,"updated":"2014-06-29 19:16:15.000000000","message":"Please define a constant string that contains this template. Then use the constant here to avoid the ugly indentation and make the code easier to read.","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":3217,"name":"Ian Wells","username":"ijw-ubuntu"},"change_message_id":"f76b2e23ddd3d456e3d2bd33cb8682506a6642de","unresolved":false,"context_lines":[{"line_number":474,"context_line":"{"},{"line_number":475,"context_line":"   AdvSendAdvert on;"},{"line_number":476,"context_line":"   MinRtrAdvInterval 3;"},{"line_number":477,"context_line":"   MaxRtrAdvInterval 10;"},{"line_number":478,"context_line":"   prefix %s"},{"line_number":479,"context_line":"   {"},{"line_number":480,"context_line":"        AdvOnLink on;"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_78093c2a","line":477,"updated":"2014-06-26 19:43:05.000000000","message":"Arbitrarily chosen constants, but I\u0027m not sure we care or necessarily need to have them configurable.  Still, it\u0027s a design choice and perhaps we should document it with a comment?","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"84270082f03f163b576db677561ddfd6b9fbffdd","unresolved":false,"context_lines":[{"line_number":474,"context_line":"{"},{"line_number":475,"context_line":"   AdvSendAdvert on;"},{"line_number":476,"context_line":"   MinRtrAdvInterval 3;"},{"line_number":477,"context_line":"   MaxRtrAdvInterval 10;"},{"line_number":478,"context_line":"   prefix %s"},{"line_number":479,"context_line":"   {"},{"line_number":480,"context_line":"        AdvOnLink on;"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_ee2bc679","line":477,"in_reply_to":"1ae5cdf2_78093c2a","updated":"2014-06-26 20:49:51.000000000","message":"If it\u0027s desirable to have them configurable, that can be a future enhancement item. In addition, the design spec should spell this out","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"6493cac4a91594b43bd455dff0e2f657a96eeca0","unresolved":false,"context_lines":[{"line_number":483,"context_line":"};"},{"line_number":484,"context_line":"\"\"\" % (interface_name, p[\u0027subnet\u0027][\u0027cidr\u0027])"},{"line_number":485,"context_line":"                else:"},{"line_number":486,"context_line":"                    conf_str \u003d \"\"\"interface %s"},{"line_number":487,"context_line":"{"},{"line_number":488,"context_line":"   AdvSendAdvert on;"},{"line_number":489,"context_line":"   MinRtrAdvInterval 3;"}],"source_content_type":"text/x-python","patch_set":2,"id":"dab895d6_e5f541cc","line":486,"updated":"2014-06-29 19:16:15.000000000","message":"Ditto, use a constant string for this template.","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":3217,"name":"Ian Wells","username":"ijw-ubuntu"},"change_message_id":"f76b2e23ddd3d456e3d2bd33cb8682506a6642de","unresolved":false,"context_lines":[{"line_number":496,"context_line":"        lnx_utils.replace_file(ra_cfg_file, buf.getvalue())"},{"line_number":497,"context_line":""},{"line_number":498,"context_line":"        if pid:"},{"line_number":499,"context_line":"            cmd \u003d [\u0027cat\u0027, \u0027/proc/%d/cmdline\u0027 % pid]"},{"line_number":500,"context_line":"            ip_wrapper \u003d ip_lib.IPWrapper(self.root_helper,"},{"line_number":501,"context_line":"                                          namespace\u003dri.ns_name)"},{"line_number":502,"context_line":"            out \u003d ip_wrapper.netns.execute(cmd, check_exit_code\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_b83584dc","line":499,"updated":"2014-06-26 19:43:05.000000000","message":"If you are reading the pidfile to get the PID, there\u0027s no necessity to check the command line of the process to find out if it\u0027s the right process.  Also, as you\u0027re witnessing, you\u0027re violating priv separation to even get at the cmdline, and you\u0027re using some very Linux-specific and setup-specific techniques here to get hold of the cmdline file.  Do you have a reason for the belt and braces approach or can we just use the PID and assume it\u0027s correct?","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"84270082f03f163b576db677561ddfd6b9fbffdd","unresolved":false,"context_lines":[{"line_number":496,"context_line":"        lnx_utils.replace_file(ra_cfg_file, buf.getvalue())"},{"line_number":497,"context_line":""},{"line_number":498,"context_line":"        if pid:"},{"line_number":499,"context_line":"            cmd \u003d [\u0027cat\u0027, \u0027/proc/%d/cmdline\u0027 % pid]"},{"line_number":500,"context_line":"            ip_wrapper \u003d ip_lib.IPWrapper(self.root_helper,"},{"line_number":501,"context_line":"                                          namespace\u003dri.ns_name)"},{"line_number":502,"context_line":"            out \u003d ip_wrapper.netns.execute(cmd, check_exit_code\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_ae3c5ec0","line":499,"in_reply_to":"1ae5cdf2_b83584dc","updated":"2014-06-26 20:49:51.000000000","message":"if you check dhcp.py, this is actually handled similarly.","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":3217,"name":"Ian Wells","username":"ijw-ubuntu"},"change_message_id":"f76b2e23ddd3d456e3d2bd33cb8682506a6642de","unresolved":false,"context_lines":[{"line_number":501,"context_line":"                                          namespace\u003dri.ns_name)"},{"line_number":502,"context_line":"            out \u003d ip_wrapper.netns.execute(cmd, check_exit_code\u003dFalse)"},{"line_number":503,"context_line":""},{"line_number":504,"context_line":"            if ra_cfg_file in out:"},{"line_number":505,"context_line":"                try:"},{"line_number":506,"context_line":"                    cmd \u003d [\u0027kill\u0027, pid]"},{"line_number":507,"context_line":"                    ip_wrapper \u003d ip_lib.IPWrapper(self.root_helper,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_3851740e","line":504,"updated":"2014-06-26 19:43:05.000000000","message":"We surely have better process management than this implemented somewhere else within Neutron.  How are we managing dnsmasq processes?","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":3217,"name":"Ian Wells","username":"ijw-ubuntu"},"change_message_id":"f76b2e23ddd3d456e3d2bd33cb8682506a6642de","unresolved":false,"context_lines":[{"line_number":503,"context_line":""},{"line_number":504,"context_line":"            if ra_cfg_file in out:"},{"line_number":505,"context_line":"                try:"},{"line_number":506,"context_line":"                    cmd \u003d [\u0027kill\u0027, pid]"},{"line_number":507,"context_line":"                    ip_wrapper \u003d ip_lib.IPWrapper(self.root_helper,"},{"line_number":508,"context_line":"                                                  namespace\u003dri.ns_name)"},{"line_number":509,"context_line":"                    ip_wrapper.netns.execute(cmd, check_exit_code\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_5852c012","line":506,"updated":"2014-06-26 19:43:05.000000000","message":"os.kill - and SIGHUP may be sufficient to get it to reread its file. without a kill/restart.  (Hard to say, the radvd manpage is nonspecific on the subject.)","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"84270082f03f163b576db677561ddfd6b9fbffdd","unresolved":false,"context_lines":[{"line_number":503,"context_line":""},{"line_number":504,"context_line":"            if ra_cfg_file in out:"},{"line_number":505,"context_line":"                try:"},{"line_number":506,"context_line":"                    cmd \u003d [\u0027kill\u0027, pid]"},{"line_number":507,"context_line":"                    ip_wrapper \u003d ip_lib.IPWrapper(self.root_helper,"},{"line_number":508,"context_line":"                                                  namespace\u003dri.ns_name)"},{"line_number":509,"context_line":"                    ip_wrapper.netns.execute(cmd, check_exit_code\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_0e7392b4","line":506,"in_reply_to":"1ae5cdf2_5852c012","updated":"2014-06-26 20:49:51.000000000","message":"for radvd, kill seems to be the way to go","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":7183,"name":"Xu Han Peng","email":"xuhanp@linux.vnet.ibm.com","username":"xuhanp"},"change_message_id":"5c35c916e7bc7fb5da13a9a2e10942973768b79f","unresolved":false,"context_lines":[{"line_number":424,"context_line":"        ri.iptables_manager.defer_apply_on()"},{"line_number":425,"context_line":"        ex_gw_port \u003d self._get_ex_gw_port(ri)"},{"line_number":426,"context_line":"        internal_ports \u003d ri.router.get(l3_constants.INTERFACE_KEY, [])"},{"line_number":427,"context_line":"        ra.enable_ipv6_ra(ri.router_id,"},{"line_number":428,"context_line":"                          ri.ns_name,"},{"line_number":429,"context_line":"                          internal_ports,"},{"line_number":430,"context_line":"                          self.get_internal_device_name,"}],"source_content_type":"text/x-python","patch_set":7,"id":"baada198_f6ab59d3","line":427,"updated":"2014-07-15 06:55:21.000000000","message":"Do you think we should consider only update IPv6 radvd conf file and process when there is a new ipv6 port added or deleted, or updated? For example, when a new IPv4 port added to the router (which has IPv6 port before update) there is no need to reload radvd process.","commit_id":"c582e1464001900acdf152ed690c2a773dc691b8"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"2609095bfaedd277b69e7510a8434ba4980ca627","unresolved":false,"context_lines":[{"line_number":424,"context_line":"        ri.iptables_manager.defer_apply_on()"},{"line_number":425,"context_line":"        ex_gw_port \u003d self._get_ex_gw_port(ri)"},{"line_number":426,"context_line":"        internal_ports \u003d ri.router.get(l3_constants.INTERFACE_KEY, [])"},{"line_number":427,"context_line":"        ra.enable_ipv6_ra(ri.router_id,"},{"line_number":428,"context_line":"                          ri.ns_name,"},{"line_number":429,"context_line":"                          internal_ports,"},{"line_number":430,"context_line":"                          self.get_internal_device_name,"}],"source_content_type":"text/x-python","patch_set":7,"id":"baada198_420343be","line":427,"in_reply_to":"baada198_850ad71d","updated":"2014-07-15 19:34:56.000000000","message":"yea, this can be done.","commit_id":"c582e1464001900acdf152ed690c2a773dc691b8"},{"author":{"_account_id":105,"name":"Kyle Mestery","email":"mestery@mestery.com","username":"mestery"},"change_message_id":"382ac9648e00de7eb7ac1bfeda734f340a707f19","unresolved":false,"context_lines":[{"line_number":424,"context_line":"        ri.iptables_manager.defer_apply_on()"},{"line_number":425,"context_line":"        ex_gw_port \u003d self._get_ex_gw_port(ri)"},{"line_number":426,"context_line":"        internal_ports \u003d ri.router.get(l3_constants.INTERFACE_KEY, [])"},{"line_number":427,"context_line":"        ra.enable_ipv6_ra(ri.router_id,"},{"line_number":428,"context_line":"                          ri.ns_name,"},{"line_number":429,"context_line":"                          internal_ports,"},{"line_number":430,"context_line":"                          self.get_internal_device_name,"}],"source_content_type":"text/x-python","patch_set":7,"id":"baada198_850ad71d","line":427,"in_reply_to":"baada198_f6ab59d3","updated":"2014-07-15 13:32:25.000000000","message":"I agree with Xu Han\u0027s comments here, we should isolate updating the file to when new ports are added/deleted/updated.","commit_id":"c582e1464001900acdf152ed690c2a773dc691b8"}],"neutron/agent/linux/external_process.py":[{"author":{"_account_id":3217,"name":"Ian Wells","username":"ijw-ubuntu"},"change_message_id":"2b081c4ac664e933f7e817ed2d942f40435fcf22","unresolved":false,"context_lines":[{"line_number":70,"context_line":"                utils.remove_conf_file(self.conf.external_pids,"},{"line_number":71,"context_line":"                                       self.uuid,"},{"line_number":72,"context_line":"                                       self.service_pid_fname)"},{"line_number":73,"context_line":"        elif pid:"},{"line_number":74,"context_line":"            LOG.debug(_(\u0027Process for %(uuid)s pid %(pid)d is stale, ignoring \u0027"},{"line_number":75,"context_line":"                        \u0027command\u0027), {\u0027uuid\u0027: self.uuid, \u0027pid\u0027: pid})"},{"line_number":76,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_36e42a3b","line":73,"updated":"2014-07-13 14:43:48.000000000","message":"If you try and reload a stale PID (because it\u0027s a crashed process) that\u0027s bad.  Also, what\u0027s restarting the process in the case of problems?  (These may be simply pre-existing issues with this class, rather than problems with your patch.  I haven\u0027t checked how it\u0027s used elsewhere.)","commit_id":"873ac1f0daf13cbf6c2c0e82daba75df32cbcd39"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"bada9abe3873cff59717107c04476631ed632d12","unresolved":false,"context_lines":[{"line_number":70,"context_line":"                utils.remove_conf_file(self.conf.external_pids,"},{"line_number":71,"context_line":"                                       self.uuid,"},{"line_number":72,"context_line":"                                       self.service_pid_fname)"},{"line_number":73,"context_line":"        elif pid:"},{"line_number":74,"context_line":"            LOG.debug(_(\u0027Process for %(uuid)s pid %(pid)d is stale, ignoring \u0027"},{"line_number":75,"context_line":"                        \u0027command\u0027), {\u0027uuid\u0027: self.uuid, \u0027pid\u0027: pid})"},{"line_number":76,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_c2ce735b","line":73,"in_reply_to":"baada198_36e42a3b","updated":"2014-07-15 19:39:18.000000000","message":"yes, these are preexisting.","commit_id":"873ac1f0daf13cbf6c2c0e82daba75df32cbcd39"},{"author":{"_account_id":105,"name":"Kyle Mestery","email":"mestery@mestery.com","username":"mestery"},"change_message_id":"382ac9648e00de7eb7ac1bfeda734f340a707f19","unresolved":false,"context_lines":[{"line_number":37,"context_line":"    Note: The manager expects uuid to be in cmdline."},{"line_number":38,"context_line":"    \"\"\""},{"line_number":39,"context_line":"    def __init__(self, conf, uuid, root_helper\u003d\u0027sudo\u0027,"},{"line_number":40,"context_line":"                 namespace\u003dNone, service\u003d\u0027\u0027):"},{"line_number":41,"context_line":"        self.conf \u003d conf"},{"line_number":42,"context_line":"        self.uuid \u003d uuid"},{"line_number":43,"context_line":"        self.root_helper \u003d root_helper"}],"source_content_type":"text/x-python","patch_set":7,"id":"baada198_85581732","line":40,"updated":"2014-07-15 13:32:25.000000000","message":"service\u003dNone","commit_id":"c582e1464001900acdf152ed690c2a773dc691b8"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"2609095bfaedd277b69e7510a8434ba4980ca627","unresolved":false,"context_lines":[{"line_number":37,"context_line":"    Note: The manager expects uuid to be in cmdline."},{"line_number":38,"context_line":"    \"\"\""},{"line_number":39,"context_line":"    def __init__(self, conf, uuid, root_helper\u003d\u0027sudo\u0027,"},{"line_number":40,"context_line":"                 namespace\u003dNone, service\u003d\u0027\u0027):"},{"line_number":41,"context_line":"        self.conf \u003d conf"},{"line_number":42,"context_line":"        self.uuid \u003d uuid"},{"line_number":43,"context_line":"        self.root_helper \u003d root_helper"}],"source_content_type":"text/x-python","patch_set":7,"id":"baada198_c2cbd371","line":40,"in_reply_to":"baada198_85581732","updated":"2014-07-15 19:34:56.000000000","message":"Done","commit_id":"c582e1464001900acdf152ed690c2a773dc691b8"}],"neutron/agent/linux/ra.py":[{"author":{"_account_id":7183,"name":"Xu Han Peng","email":"xuhanp@linux.vnet.ibm.com","username":"xuhanp"},"change_message_id":"8e317e7a11b696172c061cf557a5b008868ad403","unresolved":false,"context_lines":[{"line_number":57,"context_line":"\"\"\""},{"line_number":58,"context_line":""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"def _is_slaac(addr_mode):"},{"line_number":61,"context_line":"    return (addr_mode \u003d\u003d constants.IPV6_SLAAC or"},{"line_number":62,"context_line":"            addr_mode \u003d\u003d constants.DHCPV6_STATELESS)"},{"line_number":63,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"baada198_cecaae19","line":60,"updated":"2014-07-11 03:32:33.000000000","message":"I think we should use ipv6_ra_mode instead of ipv6_address_mode. Is there any reason that this has been changed from last patch?\n\nthere are cases where ipv6_ra_mode is None and ipv6_address_mode is slaac, in this case, the RA can be obtained from external router.","commit_id":"018dd27438cdb79c33c86e2241748e3067a4b2d1"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"0e98e49993e463bf679af00eb79d98d8f2236321","unresolved":false,"context_lines":[{"line_number":57,"context_line":"\"\"\""},{"line_number":58,"context_line":""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"def _is_slaac(addr_mode):"},{"line_number":61,"context_line":"    return (addr_mode \u003d\u003d constants.IPV6_SLAAC or"},{"line_number":62,"context_line":"            addr_mode \u003d\u003d constants.DHCPV6_STATELESS)"},{"line_number":63,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"baada198_2f5f9a42","line":60,"in_reply_to":"baada198_cecaae19","updated":"2014-07-11 11:49:43.000000000","message":"I was trying to follow the routine _check_if_subnet_uses_eui64() in db_base_plugin_v2.py. But as you pointed out, it also covers the provider ra case. \n\nI am actually confused. But I can change it to ra_mode \u003d\u003d slaac or ra_mode \u003d\u003d stateless. Would that work?\n\nI guess this page https://www.dropbox.com/s/9bojvv9vywsz8sd/IPv6%20Two%20Modes%20v3.0.pdf is still valid. In that case, when both modes are off, it seems that slaac address needs to be calculated as well. therefore _check_if_subnet_uses_eui64() is missing a case.","commit_id":"018dd27438cdb79c33c86e2241748e3067a4b2d1"},{"author":{"_account_id":7183,"name":"Xu Han Peng","email":"xuhanp@linux.vnet.ibm.com","username":"xuhanp"},"change_message_id":"8e317e7a11b696172c061cf557a5b008868ad403","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    for p in router_ports:"},{"line_number":72,"context_line":"        if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":73,"context_line":"            interface_name \u003d dev_name_helper(p[\u0027id\u0027])"},{"line_number":74,"context_line":"            if _is_slaac(p[\u0027subnet\u0027][\u0027ipv6_address_mode\u0027]):"},{"line_number":75,"context_line":"                conf_str \u003d prefix_fmt % (interface_name,"},{"line_number":76,"context_line":"                                         p[\u0027subnet\u0027][\u0027cidr\u0027])"},{"line_number":77,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"baada198_4ed69ec4","line":74,"updated":"2014-07-11 03:32:33.000000000","message":"Same as above. We should use ipv6_ra_mode here.","commit_id":"018dd27438cdb79c33c86e2241748e3067a4b2d1"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"0e98e49993e463bf679af00eb79d98d8f2236321","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    for p in router_ports:"},{"line_number":72,"context_line":"        if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":73,"context_line":"            interface_name \u003d dev_name_helper(p[\u0027id\u0027])"},{"line_number":74,"context_line":"            if _is_slaac(p[\u0027subnet\u0027][\u0027ipv6_address_mode\u0027]):"},{"line_number":75,"context_line":"                conf_str \u003d prefix_fmt % (interface_name,"},{"line_number":76,"context_line":"                                         p[\u0027subnet\u0027][\u0027cidr\u0027])"},{"line_number":77,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"baada198_4f5cce4b","line":74,"in_reply_to":"baada198_4ed69ec4","updated":"2014-07-11 11:49:43.000000000","message":"Done","commit_id":"018dd27438cdb79c33c86e2241748e3067a4b2d1"},{"author":{"_account_id":105,"name":"Kyle Mestery","email":"mestery@mestery.com","username":"mestery"},"change_message_id":"382ac9648e00de7eb7ac1bfeda734f340a707f19","unresolved":false,"context_lines":[{"line_number":93,"context_line":"                                            router_ns,"},{"line_number":94,"context_line":"                                            \u0027radvd\u0027)"},{"line_number":95,"context_line":"    radvd.enable(callback, True)"},{"line_number":96,"context_line":"    LOG.debug(_(\"radvd enabled for router %s\"), router_id)"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":""},{"line_number":99,"context_line":"def enable_ipv6_ra(router_id, router_ns, router_ports,"}],"source_content_type":"text/x-python","patch_set":7,"id":"baada198_0589676e","line":96,"updated":"2014-07-15 13:32:25.000000000","message":"Please don\u0027t translate debug logs. Also on lines 111 and 124.","commit_id":"c582e1464001900acdf152ed690c2a773dc691b8"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"2609095bfaedd277b69e7510a8434ba4980ca627","unresolved":false,"context_lines":[{"line_number":93,"context_line":"                                            router_ns,"},{"line_number":94,"context_line":"                                            \u0027radvd\u0027)"},{"line_number":95,"context_line":"    radvd.enable(callback, True)"},{"line_number":96,"context_line":"    LOG.debug(_(\"radvd enabled for router %s\"), router_id)"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":""},{"line_number":99,"context_line":"def enable_ipv6_ra(router_id, router_ns, router_ports,"}],"source_content_type":"text/x-python","patch_set":7,"id":"baada198_8d8671da","line":96,"in_reply_to":"baada198_0589676e","updated":"2014-07-15 19:34:56.000000000","message":"Done","commit_id":"c582e1464001900acdf152ed690c2a773dc691b8"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"61952bb3b21170028f80b7fdf5b4b07948075463","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":104,"context_line":"            has_ipv6 \u003d True"},{"line_number":105,"context_line":"            break"},{"line_number":106,"context_line":"    if not has_ipv6:"},{"line_number":107,"context_line":"        # Kill the daemon if it\u0027s running"},{"line_number":108,"context_line":"        disable_ipv6_ra(router_id, router_ns, root_helper)"},{"line_number":109,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_ab34a1df","line":106,"updated":"2014-07-17 15:58:47.000000000","message":"You can remove has_ipv6 from 101 and 104, and here do:\n    else:\n        # Kill the daemon if it\u0027s running\n        ...","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"7dff7e2d1940c1dda7d15d69635dea97620b1e8d","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        if netaddr.IPNetwork(p[\u0027subnet\u0027][\u0027cidr\u0027]).version \u003d\u003d 6:"},{"line_number":104,"context_line":"            has_ipv6 \u003d True"},{"line_number":105,"context_line":"            break"},{"line_number":106,"context_line":"    if not has_ipv6:"},{"line_number":107,"context_line":"        # Kill the daemon if it\u0027s running"},{"line_number":108,"context_line":"        disable_ipv6_ra(router_id, router_ns, root_helper)"},{"line_number":109,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_6b57d426","line":106,"in_reply_to":"baada198_ab34a1df","updated":"2014-07-18 04:56:35.000000000","message":"Done.","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"}],"neutron/tests/unit/test_l3_agent.py":[{"author":{"_account_id":4656,"name":"Sean M. Collins","email":"sean@coreitpro.com","username":"scollins"},"change_message_id":"9f1083b2a8d687ca57b290ea7f011d442ec1f0c2","unresolved":false,"context_lines":[{"line_number":697,"context_line":"             \u0027mac_address\u0027: \u0027ca:fe:de:ad:be:ef\u0027,"},{"line_number":698,"context_line":"             \u0027subnet\u0027: {\u0027cidr\u0027: \u0027fd00::/64\u0027,"},{"line_number":699,"context_line":"                        \u0027gateway_ip\u0027: \u0027fd00::1\u0027},"},{"line_number":700,"context_line":"             \u0027ipv6-ra-mode\u0027: \u0027\u0027})"},{"line_number":701,"context_line":"        # Reassign the router object to RouterInfo"},{"line_number":702,"context_line":"        ri.router \u003d router"},{"line_number":703,"context_line":"        with mock.patch.object(agent,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_ef8aed63","line":700,"updated":"2014-06-26 15:46:20.000000000","message":"\"ipv6_ra_mode\" - underscores, not dashes","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":3217,"name":"Ian Wells","username":"ijw-ubuntu"},"change_message_id":"f76b2e23ddd3d456e3d2bd33cb8682506a6642de","unresolved":false,"context_lines":[{"line_number":697,"context_line":"             \u0027mac_address\u0027: \u0027ca:fe:de:ad:be:ef\u0027,"},{"line_number":698,"context_line":"             \u0027subnet\u0027: {\u0027cidr\u0027: \u0027fd00::/64\u0027,"},{"line_number":699,"context_line":"                        \u0027gateway_ip\u0027: \u0027fd00::1\u0027},"},{"line_number":700,"context_line":"             \u0027ipv6-ra-mode\u0027: \u0027\u0027})"},{"line_number":701,"context_line":"        # Reassign the router object to RouterInfo"},{"line_number":702,"context_line":"        ri.router \u003d router"},{"line_number":703,"context_line":"        with mock.patch.object(agent,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_1804b8c4","line":700,"in_reply_to":"1ae5cdf2_ef8aed63","updated":"2014-06-26 19:43:05.000000000","message":"... if a flawed test passes what does that say about the test...","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"84270082f03f163b576db677561ddfd6b9fbffdd","unresolved":false,"context_lines":[{"line_number":697,"context_line":"             \u0027mac_address\u0027: \u0027ca:fe:de:ad:be:ef\u0027,"},{"line_number":698,"context_line":"             \u0027subnet\u0027: {\u0027cidr\u0027: \u0027fd00::/64\u0027,"},{"line_number":699,"context_line":"                        \u0027gateway_ip\u0027: \u0027fd00::1\u0027},"},{"line_number":700,"context_line":"             \u0027ipv6-ra-mode\u0027: \u0027\u0027})"},{"line_number":701,"context_line":"        # Reassign the router object to RouterInfo"},{"line_number":702,"context_line":"        ri.router \u003d router"},{"line_number":703,"context_line":"        with mock.patch.object(agent,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_eeb58619","line":700,"in_reply_to":"1ae5cdf2_ef8aed63","updated":"2014-06-26 20:49:51.000000000","message":"thanks for pointing it out. I think this change is not needed.","commit_id":"f13a6357048e1a2c7efb3a18e9cb22c4a779815f"},{"author":{"_account_id":3217,"name":"Ian Wells","username":"ijw-ubuntu"},"change_message_id":"2b081c4ac664e933f7e817ed2d942f40435fcf22","unresolved":false,"context_lines":[{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def setUp(self):"},{"line_number":43,"context_line":"        super(TestBasicRouterOperations, self).setUp()"},{"line_number":44,"context_line":"        self.conf \u003d agent_config.setup_conf()"},{"line_number":45,"context_line":"        self.conf.register_opts(base_config.core_opts)"},{"line_number":46,"context_line":"        self.conf.register_opts(l3_agent.L3NATAgent.OPTS)"},{"line_number":47,"context_line":"        agent_config.register_interface_driver_opts_helper(self.conf)"}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_16c2c684","line":44,"updated":"2014-07-13 14:43:48.000000000","message":"Not clear why this changed.","commit_id":"873ac1f0daf13cbf6c2c0e82daba75df32cbcd39"},{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"bada9abe3873cff59717107c04476631ed632d12","unresolved":false,"context_lines":[{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def setUp(self):"},{"line_number":43,"context_line":"        super(TestBasicRouterOperations, self).setUp()"},{"line_number":44,"context_line":"        self.conf \u003d agent_config.setup_conf()"},{"line_number":45,"context_line":"        self.conf.register_opts(base_config.core_opts)"},{"line_number":46,"context_line":"        self.conf.register_opts(l3_agent.L3NATAgent.OPTS)"},{"line_number":47,"context_line":"        agent_config.register_interface_driver_opts_helper(self.conf)"}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_c2c75345","line":44,"in_reply_to":"baada198_16c2c684","updated":"2014-07-15 19:39:18.000000000","message":"one of the config items is registered in that routine, and needed in the tests","commit_id":"873ac1f0daf13cbf6c2c0e82daba75df32cbcd39"},{"author":{"_account_id":4656,"name":"Sean M. Collins","email":"sean@coreitpro.com","username":"scollins"},"change_message_id":"5696d9949d92647b9e0cb872c67986578dd1be53","unresolved":false,"context_lines":[{"line_number":636,"context_line":"        self._verify_snat_rules(nat_rules_delta, router)"},{"line_number":637,"context_line":"        self.assertEqual(self.send_arp.call_count, 1)"},{"line_number":638,"context_line":""},{"line_number":639,"context_line":"    @staticmethod"},{"line_number":640,"context_line":"    def _router_append_interface_ipv4(router):"},{"line_number":641,"context_line":"        # Add an interface and reprocess"},{"line_number":642,"context_line":"        router[l3_constants.INTERFACE_KEY].append("}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_50b4e234","line":639,"updated":"2014-07-16 00:53:55.000000000","message":"Do these methods need to be static? I only see them being called by the class, in the \"self\" context.","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":4656,"name":"Sean M. Collins","email":"sean@coreitpro.com","username":"scollins"},"change_message_id":"966c7581bcfdbfdf674b7a0665101c74307b555f","unresolved":false,"context_lines":[{"line_number":636,"context_line":"        self._verify_snat_rules(nat_rules_delta, router)"},{"line_number":637,"context_line":"        self.assertEqual(self.send_arp.call_count, 1)"},{"line_number":638,"context_line":""},{"line_number":639,"context_line":"    @staticmethod"},{"line_number":640,"context_line":"    def _router_append_interface_ipv4(router):"},{"line_number":641,"context_line":"        # Add an interface and reprocess"},{"line_number":642,"context_line":"        router[l3_constants.INTERFACE_KEY].append("}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_42abea88","line":639,"in_reply_to":"baada198_1ea8bb1f","updated":"2014-07-17 15:14:25.000000000","message":"https://docs.python.org/2/library/functions.html#staticmethod\n\nYou add a static method to a class if you are going to invoke it from the class, not an instance of the class.\n\nSuch as:\n\nTestBasicRouterOperations._router_append_interface_ipv4(router)\n\nBut I only see references to self._router_append_interface_ipv4(router) in this file","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"a961d7c4198002f2dd207d192130557b630ef7be","unresolved":false,"context_lines":[{"line_number":636,"context_line":"        self._verify_snat_rules(nat_rules_delta, router)"},{"line_number":637,"context_line":"        self.assertEqual(self.send_arp.call_count, 1)"},{"line_number":638,"context_line":""},{"line_number":639,"context_line":"    @staticmethod"},{"line_number":640,"context_line":"    def _router_append_interface_ipv4(router):"},{"line_number":641,"context_line":"        # Add an interface and reprocess"},{"line_number":642,"context_line":"        router[l3_constants.INTERFACE_KEY].append("}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_a21f666f","line":639,"in_reply_to":"baada198_42abea88","updated":"2014-07-17 15:23:59.000000000","message":"Maybe you are thinking about a @classmethod? Static methods are just methods that are members of a class that do not need to access any of the properties of the class.","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"61952bb3b21170028f80b7fdf5b4b07948075463","unresolved":false,"context_lines":[{"line_number":636,"context_line":"        self._verify_snat_rules(nat_rules_delta, router)"},{"line_number":637,"context_line":"        self.assertEqual(self.send_arp.call_count, 1)"},{"line_number":638,"context_line":""},{"line_number":639,"context_line":"    @staticmethod"},{"line_number":640,"context_line":"    def _router_append_interface_ipv4(router):"},{"line_number":641,"context_line":"        # Add an interface and reprocess"},{"line_number":642,"context_line":"        router[l3_constants.INTERFACE_KEY].append("}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_856c2c38","line":639,"in_reply_to":"baada198_42abea88","updated":"2014-07-17 15:58:47.000000000","message":"With a static method, you can call it with the class name or you can call it with an instance (and Python will only use the Class part of the instance).\n\nSubjectively, using the class name is a bit more descriptive to me. Alternately, you could just put these at the top of the file (outside the class), if desired.","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"0acb6eac64e03781b0218b7bb8976c8291af081b","unresolved":false,"context_lines":[{"line_number":636,"context_line":"        self._verify_snat_rules(nat_rules_delta, router)"},{"line_number":637,"context_line":"        self.assertEqual(self.send_arp.call_count, 1)"},{"line_number":638,"context_line":""},{"line_number":639,"context_line":"    @staticmethod"},{"line_number":640,"context_line":"    def _router_append_interface_ipv4(router):"},{"line_number":641,"context_line":"        # Add an interface and reprocess"},{"line_number":642,"context_line":"        router[l3_constants.INTERFACE_KEY].append("}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_8b399dda","line":639,"in_reply_to":"baada198_45e8547c","updated":"2014-07-17 16:06:35.000000000","message":"My usage of static methods aligns with https://julien.danjou.info/blog/2013/guide-python-static-class-abstract-methods\n\nIn my opinion _prepare_router_data() should be static too. It is called from many test cases so making it static would save many bound-method instantiations. But that is an unrelated change.","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"8c42c886f96e962a1a04c3d4cca2f76665cfbe32","unresolved":false,"context_lines":[{"line_number":636,"context_line":"        self._verify_snat_rules(nat_rules_delta, router)"},{"line_number":637,"context_line":"        self.assertEqual(self.send_arp.call_count, 1)"},{"line_number":638,"context_line":""},{"line_number":639,"context_line":"    @staticmethod"},{"line_number":640,"context_line":"    def _router_append_interface_ipv4(router):"},{"line_number":641,"context_line":"        # Add an interface and reprocess"},{"line_number":642,"context_line":"        router[l3_constants.INTERFACE_KEY].append("}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_1ea8bb1f","line":639,"in_reply_to":"baada198_50b4e234","updated":"2014-07-16 03:12:46.000000000","message":"I am sorry, I don\u0027t understand the question. (Maybe I need to get some sleep.) How do you think these two methods should be declared?","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"7dff7e2d1940c1dda7d15d69635dea97620b1e8d","unresolved":false,"context_lines":[{"line_number":636,"context_line":"        self._verify_snat_rules(nat_rules_delta, router)"},{"line_number":637,"context_line":"        self.assertEqual(self.send_arp.call_count, 1)"},{"line_number":638,"context_line":""},{"line_number":639,"context_line":"    @staticmethod"},{"line_number":640,"context_line":"    def _router_append_interface_ipv4(router):"},{"line_number":641,"context_line":"        # Add an interface and reprocess"},{"line_number":642,"context_line":"        router[l3_constants.INTERFACE_KEY].append("}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_cb4c60a0","line":639,"in_reply_to":"baada198_856c2c38","updated":"2014-07-18 04:56:35.000000000","message":"I ended up putting them outside the class so I could reuse them in the functional tests.","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":4656,"name":"Sean M. Collins","email":"sean@coreitpro.com","username":"scollins"},"change_message_id":"c1238341b171ee7d5d485c5322bc11c2acfc3d07","unresolved":false,"context_lines":[{"line_number":636,"context_line":"        self._verify_snat_rules(nat_rules_delta, router)"},{"line_number":637,"context_line":"        self.assertEqual(self.send_arp.call_count, 1)"},{"line_number":638,"context_line":""},{"line_number":639,"context_line":"    @staticmethod"},{"line_number":640,"context_line":"    def _router_append_interface_ipv4(router):"},{"line_number":641,"context_line":"        # Add an interface and reprocess"},{"line_number":642,"context_line":"        router[l3_constants.INTERFACE_KEY].append("}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_45e8547c","line":639,"in_reply_to":"baada198_a21f666f","updated":"2014-07-17 15:36:04.000000000","message":"No, I am not thinking of @classmethod. There are other methods in this class that do not access any of the properties of the class (_prepare_router_data) but are not decorated with the @staticmethod decorator.","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":4656,"name":"Sean M. Collins","email":"sean@coreitpro.com","username":"scollins"},"change_message_id":"5696d9949d92647b9e0cb872c67986578dd1be53","unresolved":false,"context_lines":[{"line_number":649,"context_line":"             \u0027subnet\u0027: {\u0027cidr\u0027: \u002735.4.1.0/24\u0027,"},{"line_number":650,"context_line":"                        \u0027gateway_ip\u0027: \u002735.4.1.1\u0027}})"},{"line_number":651,"context_line":""},{"line_number":652,"context_line":"    @staticmethod"},{"line_number":653,"context_line":"    def _router_append_interface_ipv6(router, ra_mode\u003dNone, addr_mode\u003dNone):"},{"line_number":654,"context_line":"        router[l3_constants.INTERFACE_KEY].append("},{"line_number":655,"context_line":"            {\u0027id\u0027: _uuid(),"}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_b07a9e75","line":652,"updated":"2014-07-16 00:53:55.000000000","message":"See above comment","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"61952bb3b21170028f80b7fdf5b4b07948075463","unresolved":false,"context_lines":[{"line_number":650,"context_line":"                        \u0027gateway_ip\u0027: \u002735.4.1.1\u0027}})"},{"line_number":651,"context_line":""},{"line_number":652,"context_line":"    @staticmethod"},{"line_number":653,"context_line":"    def _router_append_interface_ipv6(router, ra_mode\u003dNone, addr_mode\u003dNone):"},{"line_number":654,"context_line":"        router[l3_constants.INTERFACE_KEY].append("},{"line_number":655,"context_line":"            {\u0027id\u0027: _uuid(),"},{"line_number":656,"context_line":"             \u0027network_id\u0027: _uuid(),"}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_b0beccfe","line":653,"updated":"2014-07-17 15:58:47.000000000","message":"Since these two methods are very similar, consider using one method and passing a flag for IPv4/v6.","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"0acb6eac64e03781b0218b7bb8976c8291af081b","unresolved":false,"context_lines":[{"line_number":650,"context_line":"                        \u0027gateway_ip\u0027: \u002735.4.1.1\u0027}})"},{"line_number":651,"context_line":""},{"line_number":652,"context_line":"    @staticmethod"},{"line_number":653,"context_line":"    def _router_append_interface_ipv6(router, ra_mode\u003dNone, addr_mode\u003dNone):"},{"line_number":654,"context_line":"        router[l3_constants.INTERFACE_KEY].append("},{"line_number":655,"context_line":"            {\u0027id\u0027: _uuid(),"},{"line_number":656,"context_line":"             \u0027network_id\u0027: _uuid(),"}],"source_content_type":"text/x-python","patch_set":8,"id":"baada198_2b2ed1fa","line":653,"in_reply_to":"baada198_b0beccfe","updated":"2014-07-17 16:06:35.000000000","message":"I did try that initially and it looked rather messy (harder to read). I\u0027ll do it in the next patch so let me know what you think then.","commit_id":"5238a6b1c4759b764ab85cdf6c917ae1dbffaa87"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"3975b497b29ae2b22f06ad7c5638c3700e05ed50","unresolved":false,"context_lines":[{"line_number":446,"context_line":"                self.assertIn(r.rule, expected_rules)"},{"line_number":447,"context_line":""},{"line_number":448,"context_line":"    @staticmethod"},{"line_number":449,"context_line":"    def _router_append_interface(router, count\u003d1, ip_version\u003d4,"},{"line_number":450,"context_line":"                                 ra_mode\u003dNone, addr_mode\u003dNone):"},{"line_number":451,"context_line":"        if ip_version \u003d\u003d 4:"},{"line_number":452,"context_line":"            ip_pool \u003d \u002735.4.%i.4\u0027"}],"source_content_type":"text/x-python","patch_set":10,"id":"baada198_5fd27f00","line":449,"updated":"2014-07-21 11:53:46.000000000","message":"Looks fine. It\u0027s a bit more complicated that the version specific routines you had, but that is because of the added functionality for multiple interfaces.\n\nI guess the only question here is whether you need the multi-append capability now. If not, you could simplify and just have a base IP offset as an argument.  For future, if multiple addresses are needed, a wrapper that calcs the start offset and then calls via a loop, can be used...\n\n    def _router_append_multiple_interfaces(router, count, ip_version\u003d4, ra_mode\u003dNone, addr_mode\u003dNone):\n        # Do the calculation on L462-465 to get current\n        for i in range(current, current + count):\n            self._router_append_interface(router, i, ip_version, ra_mode, addr_mode)","commit_id":"7f8ae630b87392193974dd9cb198c1165cdec93b"}]}