Thanks for the review.
On Thu, Mar 9, 2017 at 2:48 AM, Ben Pfaff <b...@ovn.org> wrote: > On Tue, Mar 07, 2017 at 07:19:45AM +0530, nusid...@redhat.com wrote: > > From: Numan Siddique <nusid...@redhat.com> > > > > OVN will generate the IPv6 address for a logical port if requested > > using the IPv6 prefix and the MAC address (as IEEE EUI64 identifier). > > To generate the IPv6 address, CMS should define the IPv6 prefix in the > > 'Logical_switch.other_config:ipv6_prefix' column. > > > > Signed-off-by: Numan Siddique <nusid...@redhat.com> > > The documentation uses <dl> for a bulleted list. Please use <ul>. > > Done. > The test uses explicit "sleep" calls. Are these needed? I guess that > --wait=hv would also work, but faster. > > It was a mistake. WIll remove it. > I found ipam_allocate_addresses() to be a little hard to follow. How > about rewriting it like this? I have not tested it beyond compiling, > but the general code flow seems more straightforward to me. > > static bool > ipam_allocate_addresses(struct ovn_datapath *od, struct ovn_port *op, > const char *addrspec) > { > if (!op->nbsp || !od->ipam_info) { > return false; > } > > /* Get or generate MAC address. */ > struct eth_addr mac; > bool dynamic_mac; > int n = 0; > if (ovs_scan(addrspec, ETH_ADDR_SCAN_FMT" dynamic%n", > ETH_ADDR_SCAN_ARGS(mac), &n) > && addrspec[n] == '\0') { > dynamic_mac = false; > } else { > uint64_t mac64 = ipam_get_unused_mac(); > if (!mac64) { > return false; > } > eth_addr_from_uint64(mac64, &mac); > dynamic_mac = true; > } > > /* Generate IPv4 address, if desirable. */ > bool dynamic_ip4 = od->ipam_info->allocated_ipv4s != NULL; > uint32_t ip4 = dynamic_ip4 ? ipam_get_unused_ip(od) : 0; > > /* Generate IPv6 address, if desirable. */ > bool dynamic_ip6 = od->ipam_info->ipv6_prefix_set; > struct in6_addr ip6; > if (dynamic_ip6) { > in6_generate_eui64(mac, &od->ipam_info->ipv6_prefix, &ip6); > } > > /* If we didn't generate anything, bail out. */ > if (!dynamic_ip4 && !dynamic_ip6) { > return false; > } > > /* Save the dynamic addresses. */ > struct ds new_addr = DS_EMPTY_INITIALIZER; > ds_put_format(&new_addr, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac)); > if (dynamic_ip4) { > ipam_insert_ip(od, ip4); > ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(htonl(ip4))); > } > if (dynamic_ip6) { > char ip6_s[INET6_ADDRSTRLEN + 1]; > ipv6_string_mapped(ip6_s, &ip6); > ds_put_format(&new_addr, " %s", ip6_s); > } > ipam_insert_mac(&mac, !dynamic_mac); > nbrec_logical_switch_port_set_dynamic_addresses(op->nbsp, > ds_cstr(&new_addr)); > ds_destroy(&new_addr); > return true; > } > Thanks for making it cleaner. > > I am a little concerned that, when no dynamic address is assigned, the > code does not clear the dynamic_addresses column. That means that, if > "dynamic" is removed from the addresses column, the former dynamic > addresses will still appear. I suspect that the code should clear > dynamic_addresses when there are no dynamic addresses. > I have addressed this in the v3 patch 1. > > Thanks, > > Ben. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev