Re: [ovs-dev] [PATCH v9] ovn-nbctl: Add LB commands.
Thanks for your tips, I fixed the memory leak(via free(ips)). The patch follows the style of the original code. So the 'const' is not used with 'may_exist', 'add_duplicate' and 'must_exist'. You can find that in files, such as 'utilities/ovs-vsctl.c', 'ovn/utilities/ovn-sbctl.c' and 'ovn/utilities/ovn-nbctl.c'. If necessary, I will submit an individual patch. > On Sep 30, 2016, at 5:30 AM, Flavio Fernandes wrote: > > comments/suggestions inline. > > Also, I'd think it would be good to run with valgrind [1] to see if there is > any error codepath > that may be causing leaks. > > -- flaviof ___ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v9] ovn-nbctl: Add LB commands.
> On Sep 29, 2016, at 5:43 AM, nickcooper-zhangtonghao
> wrote:
>
> This patch provides the command line to create a load balancer.
> You can create a load balancer independently and add it to multiple
> switches or routers. A single load balancer can have multiple vips.
> Add a name column for the load balancer. With --add-duplicate,
> the command really creates a new load balancer with a duplicate name.
> This name has no special meaning or purpose other than to provide
> convenience for human interaction with the ovn-nb database.
> This patch also provides the unit tests and the documentation.
>
> Signed-off-by: nickcooper-zhangtonghao
>
comments/suggestions inline.
Also, I'd think it would be good to run with valgrind [1] to see if there is
any error codepath
that may be causing leaks.
-- flaviof
[1]: cd ~/ovs.git && make check-valgrind TESTSUITEFLAGS='-k ovn'
> ---
> lib/packets.c | 18 ++
> lib/packets.h | 10 +
> ovn/ovn-nb.ovsschema | 5 +-
> ovn/ovn-nb.xml| 6 +
> ovn/utilities/ovn-nbctl.8.xml | 105 +
> ovn/utilities/ovn-nbctl.c | 480 +-
> tests/ovn-nbctl.at| 218 +++
> 7 files changed, 839 insertions(+), 3 deletions(-)
>
> diff --git a/lib/packets.c b/lib/packets.c
> index e4c29d5..11bb587 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -427,6 +427,24 @@ ip_parse(const char *s, ovs_be32 *ip)
> return inet_pton(AF_INET, s, ip) == 1;
> }
>
> +/* Parses string 's', which must be an IP address with a port number
> + * with ":" as a separator (e.g.: 192.168.1.2:80).
> + * Stores the IP address into '*ip' and port number to '*port'. */
> +char * OVS_WARN_UNUSED_RESULT
> +ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port)
> +{
> +int n = 0;
> +if (!ovs_scan_len(s, &n, IP_PORT_SCAN_FMT,
> +IP_PORT_SCAN_ARGS(ip, port))) {
> +return xasprintf("%s: invalid IP address or port number", s);
> +}
> +
> +if (s[n]) {
> +return xasprintf("%s: invalid IP address or port number", s);
> +}
> +return NULL;
> +}
> +
> /* Parses string 's', which must be an IP address with an optional netmask or
> * CIDR prefix length. Stores the IP address into '*ip', netmask into
> '*mask',
> * (255.255.255.255, if 's' lacks a netmask), and number of scanned characters
> diff --git a/lib/packets.h b/lib/packets.h
> index dcfcd04..21bd35c 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -537,6 +537,14 @@ mpls_lse_to_bos(ovs_be32 mpls_lse)
> &((uint8_t *) ip)[2], \
> &((uint8_t *) ip)[3]
>
> +#define IP_PORT_SCAN_FMT "%"SCNu8".%"SCNu8".%"SCNu8".%"SCNu8":%"SCNu16
> +#define IP_PORT_SCAN_ARGS(ip, port)\
> +((void) (ovs_be32) *(ip), &((uint8_t *) ip)[0]),\
> +&((uint8_t *) ip)[1], \
> +&((uint8_t *) ip)[2], \
> +&((uint8_t *) ip)[3], \
> +((void) (ovs_be16) *(port), (uint16_t *) port)
> +
> /* Returns true if 'netmask' is a CIDR netmask, that is, if it consists of N
> * high-order 1-bits and 32-N low-order 0-bits. */
> static inline bool
> @@ -558,6 +566,8 @@ ip_is_local_multicast(ovs_be32 ip)
> int ip_count_cidr_bits(ovs_be32 netmask);
> void ip_format_masked(ovs_be32 ip, ovs_be32 mask, struct ds *);
> bool ip_parse(const char *s, ovs_be32 *ip);
> +char *ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port)
> +OVS_WARN_UNUSED_RESULT;
> char *ip_parse_masked(const char *s, ovs_be32 *ip, ovs_be32 *mask)
> OVS_WARN_UNUSED_RESULT;
> char *ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen)
> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
> index b7e70aa..5f2f2bf 100644
> --- a/ovn/ovn-nb.ovsschema
> +++ b/ovn/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Northbound",
> -"version": "5.3.3",
> -"cksum": "2442952958 9945",
> +"version": "5.3.4",
> +"cksum": "1155817817 9975",
> "tables": {
> "NB_Global": {
> "columns": {
> @@ -97,6 +97,7 @@
> "isRoot": true},
> "Load_Balancer": {
> "columns": {
> + "name": {"type": "string"},
> "vips": {
> "type": {"key": "string", "value": "string",
> "min": 0, "max": "unlimited"}},
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index c45a444..b7690d0 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -676,6 +676,12 @@
> Each row represents one load balancer.
>
>
> +
> + A name for the load balancer. This name has no special meaning or
> + purpose other than to provide convenience for human interaction with
> + the ovn-nb database.
> +
> +
>
>
> A map of virtual IPv4 addresses (and an optional port number
Re: [ovs-dev] [PATCH v9] ovn-nbctl: Add LB commands.
On 29 September 2016 at 02:43, nickcooper-zhangtonghao < [email protected]> wrote: > This patch provides the command line to create a load balancer. > You can create a load balancer independently and add it to multiple > switches or routers. A single load balancer can have multiple vips. > Add a name column for the load balancer. With --add-duplicate, > the command really creates a new load balancer with a duplicate name. > This name has no special meaning or purpose other than to provide > convenience for human interaction with the ovn-nb database. > This patch also provides the unit tests and the documentation. > > Signed-off-by: nickcooper-zhangtonghao opencloud.tech> > Thank you for your patience and persistence. I will apply this in a day or two (if no one else has any other comments) > --- > lib/packets.c | 18 ++ > lib/packets.h | 10 + > ovn/ovn-nb.ovsschema | 5 +- > ovn/ovn-nb.xml| 6 + > ovn/utilities/ovn-nbctl.8.xml | 105 + > ovn/utilities/ovn-nbctl.c | 480 ++ > +++- > tests/ovn-nbctl.at| 218 +++ > 7 files changed, 839 insertions(+), 3 deletions(-) > > diff --git a/lib/packets.c b/lib/packets.c > index e4c29d5..11bb587 100644 > --- a/lib/packets.c > +++ b/lib/packets.c > @@ -427,6 +427,24 @@ ip_parse(const char *s, ovs_be32 *ip) > return inet_pton(AF_INET, s, ip) == 1; > } > > +/* Parses string 's', which must be an IP address with a port number > + * with ":" as a separator (e.g.: 192.168.1.2:80). > + * Stores the IP address into '*ip' and port number to '*port'. */ > +char * OVS_WARN_UNUSED_RESULT > +ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port) > +{ > +int n = 0; > +if (!ovs_scan_len(s, &n, IP_PORT_SCAN_FMT, > +IP_PORT_SCAN_ARGS(ip, port))) { > +return xasprintf("%s: invalid IP address or port number", s); > +} > + > +if (s[n]) { > +return xasprintf("%s: invalid IP address or port number", s); > +} > +return NULL; > +} > + > /* Parses string 's', which must be an IP address with an optional > netmask or > * CIDR prefix length. Stores the IP address into '*ip', netmask into > '*mask', > * (255.255.255.255, if 's' lacks a netmask), and number of scanned > characters > diff --git a/lib/packets.h b/lib/packets.h > index dcfcd04..21bd35c 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -537,6 +537,14 @@ mpls_lse_to_bos(ovs_be32 mpls_lse) > &((uint8_t *) ip)[2], \ > &((uint8_t *) ip)[3] > > +#define IP_PORT_SCAN_FMT "%"SCNu8".%"SCNu8".%"SCNu8".%"SCNu8":%"SCNu16 > +#define IP_PORT_SCAN_ARGS(ip, port)\ > +((void) (ovs_be32) *(ip), &((uint8_t *) ip)[0]),\ > +&((uint8_t *) ip)[1], \ > +&((uint8_t *) ip)[2], \ > +&((uint8_t *) ip)[3], \ > +((void) (ovs_be16) *(port), (uint16_t *) port) > + > /* Returns true if 'netmask' is a CIDR netmask, that is, if it consists > of N > * high-order 1-bits and 32-N low-order 0-bits. */ > static inline bool > @@ -558,6 +566,8 @@ ip_is_local_multicast(ovs_be32 ip) > int ip_count_cidr_bits(ovs_be32 netmask); > void ip_format_masked(ovs_be32 ip, ovs_be32 mask, struct ds *); > bool ip_parse(const char *s, ovs_be32 *ip); > +char *ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port) > +OVS_WARN_UNUSED_RESULT; > char *ip_parse_masked(const char *s, ovs_be32 *ip, ovs_be32 *mask) > OVS_WARN_UNUSED_RESULT; > char *ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen) > diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema > index b7e70aa..5f2f2bf 100644 > --- a/ovn/ovn-nb.ovsschema > +++ b/ovn/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > -"version": "5.3.3", > -"cksum": "2442952958 9945", > +"version": "5.3.4", > +"cksum": "1155817817 9975", > "tables": { > "NB_Global": { > "columns": { > @@ -97,6 +97,7 @@ > "isRoot": true}, > "Load_Balancer": { > "columns": { > + "name": {"type": "string"}, > "vips": { > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}, > diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml > index c45a444..b7690d0 100644 > --- a/ovn/ovn-nb.xml > +++ b/ovn/ovn-nb.xml > @@ -676,6 +676,12 @@ >Each row represents one load balancer. > > > + > + A name for the load balancer. This name has no special meaning or > + purpose other than to provide convenience for human interaction with > + the ovn-nb database. > + > + > > > A map of virtual IPv4 addresses (and an optional port number with > diff --git
