> On Jul 12, 2016, at 8:44 PM, Ben Pfaff <[email protected]> wrote:
>
> On Mon, Jul 11, 2016 at 11:56:44PM -0700, Justin Pettit wrote:
>> Supporting multiple addresses is only mildly interesting for IPv4.
>> However, it is a requirement for IPv6, which will arrive in a future
>> commit.
>>
>> This commit introduces the extract_lrp_networks() function to ovn-util.[ch].
>>
>> Signed-off-by: Justin Pettit <[email protected]>
>
> "sparse" complains. Here is the fix:
>
> diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c
> index 3e0c138..c3cb8e4 100644
> --- a/ovn/lib/ovn-util.c
> +++ b/ovn/lib/ovn-util.c
> @@ -20,7 +20,7 @@
> VLOG_DEFINE_THIS_MODULE(ovn_util);
>
> static void
> -add_ipv4_netaddr(struct lport_addresses *laddrs, uint32_t addr,
> +add_ipv4_netaddr(struct lport_addresses *laddrs, ovs_be32 addr,
> unsigned int plen)
> {
> laddrs->n_ipv4_addrs++;
Thanks for catching that.
> The documentation for the lrp-add command is confusing for a few
> reasons. First, it talks about an optional argument "peer" that isn't
> in the synopsis. I guess that this is a special case of the ability to
> set arbitrary key/value pairs, but this may not be obvious to the casual
> reader. It's also somewhat weird that --may-exist verifies that this
> doesn't change but not other key-value pairs. Second, it's probably not
> easy to a casual reader to tell how the command distinguishes "network"
> parameters from key-values. An example might help.
That's fair. I didn't really like any of the syntax that I came up for
supporting multiple addresses. I've changed the documentation to this:
[--may-exist] lrp-add router port mac network... [peer=peer]
Creates on router a new logical router port named port
with Ethernet address mac and one or more IP address/net‐
mask for each network.
The optional argument peer identifies a logical router
port that connects to this one. The following example
adds a router port with an IPv4 and IPv6 address with
peer lr1:
lrp-add lr0 lrp0 00:11:22:33:44:55 192.168.0.1/24
2001:db8::1/64 peer=lr1
It is an error if a logical router port named port
already exists, unless --may-exist is specified. Regard‐
less of --may-exist, it is an error if the existing
router port is in some logical router other than router.
Let me know if you have a better suggestion.
> The usage message for ovn-nbctl mentions the peer=PEER special case but
> not the general case. If that's intentional, I guess it's OK--a usage
> message cannot be complete, otherwise it would be a manpage.
Yes, it's intentional, since space is limited. In general, I find the whole
peer thing a little ugly, but I don't have a better suggestion at the moment.
> In ovn-nbctl, I think that the minimum valid number of arguments is
> really 4 here (since 3 will always yield an error):
> + { "lrp-add", 3, INT_MAX,
> + "ROUTER PORT MAC NETWORK... [COLUMN[:KEY]=VALUE]...",
Thanks for catching that.
> In add_ipv4_netaddr() and add_ipv6_netaddr(), usually we write "sizeof
> *object" instead of "sizeof(type)"
Done.
> In add_ipv6_netaddr(), it seems odd to bother xmallocing() a fixed size
> instead of just including an equivalent fixed-sized buffer in struct
> ipv6_netaddr.
As you suggested earlier, I've rewritten that functionality to not dynamically
allocate memory for any of these address strings.
> Acked-by: Ben Pfaff <[email protected]>
Thanks,
--Justin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
