Re: [ovs-dev] [PATCH ovn v2 2/7] northd: Refactor load balancer vip parsing.
On Tue, Nov 3, 2020 at 3:14 PM Numan Siddique wrote:
>
> On Fri, Oct 30, 2020 at 1:52 AM Dumitru Ceara wrote:
> >
> > On 10/27/20 6:16 PM, num...@ovn.org wrote:
> > > From: Numan Siddique
> > >
> > > Parsing of the load balancer VIPs is moved to a separate file - lib/lb.c.
> > > ovn-northd makes use of these functions. Upcoming patch will make use of
> > > these
> > > util functions for parsing SB Load_Balancers.
> > >
> > > Signed-off-by: Numan Siddique
> > > ---
> > > lib/automake.mk | 4 +-
> > > lib/lb.c| 236
> > > lib/lb.h| 77
> > > lib/ovn-util.c | 28 +
> > > lib/ovn-util.h | 2 +
> > > northd/ovn-northd.c | 286 +---
> > > 6 files changed, 378 insertions(+), 255 deletions(-)
> > > create mode 100644 lib/lb.c
> > > create mode 100644 lib/lb.h
> > >
> > > diff --git a/lib/automake.mk b/lib/automake.mk
> > > index f3e9c8818b..430cd11fc6 100644
> > > --- a/lib/automake.mk
> > > +++ b/lib/automake.mk
> > > @@ -23,7 +23,9 @@ lib_libovn_la_SOURCES = \
> > > lib/ovn-util.h \
> > > lib/logical-fields.c \
> > > lib/inc-proc-eng.c \
> > > - lib/inc-proc-eng.h
> > > + lib/inc-proc-eng.h \
> > > + lib/lb.c \
> > > + lib/lb.h
> > > nodist_lib_libovn_la_SOURCES = \
> > > lib/ovn-dirs.c \
> > > lib/ovn-nb-idl.c \
> > > diff --git a/lib/lb.c b/lib/lb.c
> > > new file mode 100644
> > > index 00..db2d3d552f
> > > --- /dev/null
> > > +++ b/lib/lb.c
> > > @@ -0,0 +1,236 @@
> > > +/* Copyright (c) 2020, Red Hat, Inc.
> > > + *
> > > + * Licensed under the Apache License, Version 2.0 (the "License");
> > > + * you may not use this file except in compliance with the License.
> > > + * You may obtain a copy of the License at:
> > > + *
> > > + * http://www.apache.org/licenses/LICENSE-2.0
> > > + *
> > > + * Unless required by applicable law or agreed to in writing, software
> > > + * distributed under the License is distributed on an "AS IS" BASIS,
> > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > > implied.
> > > + * See the License for the specific language governing permissions and
> > > + * limitations under the License.
> > > + */
> > > +
> > > +#include
> > > +
> > > +#include "lb.h"
> > > +#include "lib/ovn-nb-idl.h"
> > > +#include "lib/ovn-sb-idl.h"
> > > +#include "lib/ovn-util.h"
> > > +
> > > +/* OpenvSwitch lib includes. */
> > > +#include "openvswitch/vlog.h"
> > > +#include "lib/smap.h"
> > > +
> > > +VLOG_DEFINE_THIS_MODULE(lb);
> > > +
> > > +static struct ovn_lb *
> > > +ovn_lb_create(const struct smap *vips)
> > > +{
> > > +struct ovn_lb *lb = xzalloc(sizeof *lb);
> > > +
> > > +lb->n_vips = smap_count(vips);
> > > +lb->vips = xcalloc(lb->n_vips, sizeof (struct lb_vip));
> > > +struct smap_node *node;
> > > +size_t n_vips = 0;
> > > +
> > > +SMAP_FOR_EACH (node, vips) {
> > > +char *vip;
> > > +uint16_t port;
> > > +int addr_family;
> > > +
> > > +if (!ip_address_and_port_from_key(node->key, &vip, &port,
> > > + &addr_family)) {
> > > +continue;
> > > +}
> > > +
> > > +lb->vips[n_vips].vip = vip;
> > > +lb->vips[n_vips].vip_port = port;
> > > +lb->vips[n_vips].addr_family = addr_family;
> > > +lb->vips[n_vips].vip_port_str = xstrdup(node->key);
> > > +lb->vips[n_vips].backend_ips = xstrdup(node->value);
> > > +
> > > +char *tokstr = xstrdup(node->value);
> > > +char *save_ptr = NULL;
> > > +char *token;
> > > +size_t n_backends = 0;
> > > +/* Format for a backend ips : IP1:port1,IP2:port2,...". */
> > > +for (token = strtok_r(tokstr, ",", &save_ptr);
> > > +token != NULL;
> > > +token = strtok_r(NULL, ",", &save_ptr)) {
> > > +n_backends++;
> > > +}
> > > +
> > > +free(tokstr);
> > > +tokstr = xstrdup(node->value);
> > > +save_ptr = NULL;
> > > +
> > > +lb->vips[n_vips].n_backends = n_backends;
> > > +lb->vips[n_vips].backends = xcalloc(n_backends,
> > > +sizeof (struct
> > > lb_vip_backend));
> >
> > This should be 'sizeof *lb->vips[n_vips].backends'.
>
> This part of the code is moved from ovn-northd.c. We could use both -
> an expression
> or a structure in sizeof. The coding guidelines give preference to the former.
>
> >
> > > +
> >
> > Nit: It's probably fine to remove an empty line here, the one above is
> > already
> > indented enough to the right.
> >
> > > +size_t i = 0;
> > > +for (token = strtok_r(tokstr, ",", &save_ptr);
> > > +token != NULL;
> > > +token = strtok_r(NULL, ",", &save_ptr)) {
> > > +char *backend_ip;
> > > +uint16_t backend_port;
> > > +
> > > +
Re: [ovs-dev] [PATCH ovn v2 2/7] northd: Refactor load balancer vip parsing.
On Fri, Oct 30, 2020 at 1:52 AM Dumitru Ceara wrote:
>
> On 10/27/20 6:16 PM, num...@ovn.org wrote:
> > From: Numan Siddique
> >
> > Parsing of the load balancer VIPs is moved to a separate file - lib/lb.c.
> > ovn-northd makes use of these functions. Upcoming patch will make use of
> > these
> > util functions for parsing SB Load_Balancers.
> >
> > Signed-off-by: Numan Siddique
> > ---
> > lib/automake.mk | 4 +-
> > lib/lb.c| 236
> > lib/lb.h| 77
> > lib/ovn-util.c | 28 +
> > lib/ovn-util.h | 2 +
> > northd/ovn-northd.c | 286 +---
> > 6 files changed, 378 insertions(+), 255 deletions(-)
> > create mode 100644 lib/lb.c
> > create mode 100644 lib/lb.h
> >
> > diff --git a/lib/automake.mk b/lib/automake.mk
> > index f3e9c8818b..430cd11fc6 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -23,7 +23,9 @@ lib_libovn_la_SOURCES = \
> > lib/ovn-util.h \
> > lib/logical-fields.c \
> > lib/inc-proc-eng.c \
> > - lib/inc-proc-eng.h
> > + lib/inc-proc-eng.h \
> > + lib/lb.c \
> > + lib/lb.h
> > nodist_lib_libovn_la_SOURCES = \
> > lib/ovn-dirs.c \
> > lib/ovn-nb-idl.c \
> > diff --git a/lib/lb.c b/lib/lb.c
> > new file mode 100644
> > index 00..db2d3d552f
> > --- /dev/null
> > +++ b/lib/lb.c
> > @@ -0,0 +1,236 @@
> > +/* Copyright (c) 2020, Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include
> > +
> > +#include "lb.h"
> > +#include "lib/ovn-nb-idl.h"
> > +#include "lib/ovn-sb-idl.h"
> > +#include "lib/ovn-util.h"
> > +
> > +/* OpenvSwitch lib includes. */
> > +#include "openvswitch/vlog.h"
> > +#include "lib/smap.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(lb);
> > +
> > +static struct ovn_lb *
> > +ovn_lb_create(const struct smap *vips)
> > +{
> > +struct ovn_lb *lb = xzalloc(sizeof *lb);
> > +
> > +lb->n_vips = smap_count(vips);
> > +lb->vips = xcalloc(lb->n_vips, sizeof (struct lb_vip));
> > +struct smap_node *node;
> > +size_t n_vips = 0;
> > +
> > +SMAP_FOR_EACH (node, vips) {
> > +char *vip;
> > +uint16_t port;
> > +int addr_family;
> > +
> > +if (!ip_address_and_port_from_key(node->key, &vip, &port,
> > + &addr_family)) {
> > +continue;
> > +}
> > +
> > +lb->vips[n_vips].vip = vip;
> > +lb->vips[n_vips].vip_port = port;
> > +lb->vips[n_vips].addr_family = addr_family;
> > +lb->vips[n_vips].vip_port_str = xstrdup(node->key);
> > +lb->vips[n_vips].backend_ips = xstrdup(node->value);
> > +
> > +char *tokstr = xstrdup(node->value);
> > +char *save_ptr = NULL;
> > +char *token;
> > +size_t n_backends = 0;
> > +/* Format for a backend ips : IP1:port1,IP2:port2,...". */
> > +for (token = strtok_r(tokstr, ",", &save_ptr);
> > +token != NULL;
> > +token = strtok_r(NULL, ",", &save_ptr)) {
> > +n_backends++;
> > +}
> > +
> > +free(tokstr);
> > +tokstr = xstrdup(node->value);
> > +save_ptr = NULL;
> > +
> > +lb->vips[n_vips].n_backends = n_backends;
> > +lb->vips[n_vips].backends = xcalloc(n_backends,
> > +sizeof (struct
> > lb_vip_backend));
>
> This should be 'sizeof *lb->vips[n_vips].backends'.
This part of the code is moved from ovn-northd.c. We could use both -
an expression
or a structure in sizeof. The coding guidelines give preference to the former.
>
> > +
>
> Nit: It's probably fine to remove an empty line here, the one above is already
> indented enough to the right.
>
> > +size_t i = 0;
> > +for (token = strtok_r(tokstr, ",", &save_ptr);
> > +token != NULL;
> > +token = strtok_r(NULL, ",", &save_ptr)) {
> > +char *backend_ip;
> > +uint16_t backend_port;
> > +
> > +if (!ip_address_and_port_from_key(token, &backend_ip,
> > + &backend_port,
> > + &addr_family)) {
> > +continue;
> > +}
> > +
> > +lb->vips[n_vips].backends[i].ip = backend_ip;
> > +lb->vips[n_v
Re: [ovs-dev] [PATCH ovn v2 2/7] northd: Refactor load balancer vip parsing.
On 10/27/20 6:16 PM, num...@ovn.org wrote:
> From: Numan Siddique
>
> Parsing of the load balancer VIPs is moved to a separate file - lib/lb.c.
> ovn-northd makes use of these functions. Upcoming patch will make use of these
> util functions for parsing SB Load_Balancers.
>
> Signed-off-by: Numan Siddique
> ---
> lib/automake.mk | 4 +-
> lib/lb.c| 236
> lib/lb.h| 77
> lib/ovn-util.c | 28 +
> lib/ovn-util.h | 2 +
> northd/ovn-northd.c | 286 +---
> 6 files changed, 378 insertions(+), 255 deletions(-)
> create mode 100644 lib/lb.c
> create mode 100644 lib/lb.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index f3e9c8818b..430cd11fc6 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -23,7 +23,9 @@ lib_libovn_la_SOURCES = \
> lib/ovn-util.h \
> lib/logical-fields.c \
> lib/inc-proc-eng.c \
> - lib/inc-proc-eng.h
> + lib/inc-proc-eng.h \
> + lib/lb.c \
> + lib/lb.h
> nodist_lib_libovn_la_SOURCES = \
> lib/ovn-dirs.c \
> lib/ovn-nb-idl.c \
> diff --git a/lib/lb.c b/lib/lb.c
> new file mode 100644
> index 00..db2d3d552f
> --- /dev/null
> +++ b/lib/lb.c
> @@ -0,0 +1,236 @@
> +/* Copyright (c) 2020, Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +
> +#include "lb.h"
> +#include "lib/ovn-nb-idl.h"
> +#include "lib/ovn-sb-idl.h"
> +#include "lib/ovn-util.h"
> +
> +/* OpenvSwitch lib includes. */
> +#include "openvswitch/vlog.h"
> +#include "lib/smap.h"
> +
> +VLOG_DEFINE_THIS_MODULE(lb);
> +
> +static struct ovn_lb *
> +ovn_lb_create(const struct smap *vips)
> +{
> +struct ovn_lb *lb = xzalloc(sizeof *lb);
> +
> +lb->n_vips = smap_count(vips);
> +lb->vips = xcalloc(lb->n_vips, sizeof (struct lb_vip));
> +struct smap_node *node;
> +size_t n_vips = 0;
> +
> +SMAP_FOR_EACH (node, vips) {
> +char *vip;
> +uint16_t port;
> +int addr_family;
> +
> +if (!ip_address_and_port_from_key(node->key, &vip, &port,
> + &addr_family)) {
> +continue;
> +}
> +
> +lb->vips[n_vips].vip = vip;
> +lb->vips[n_vips].vip_port = port;
> +lb->vips[n_vips].addr_family = addr_family;
> +lb->vips[n_vips].vip_port_str = xstrdup(node->key);
> +lb->vips[n_vips].backend_ips = xstrdup(node->value);
> +
> +char *tokstr = xstrdup(node->value);
> +char *save_ptr = NULL;
> +char *token;
> +size_t n_backends = 0;
> +/* Format for a backend ips : IP1:port1,IP2:port2,...". */
> +for (token = strtok_r(tokstr, ",", &save_ptr);
> +token != NULL;
> +token = strtok_r(NULL, ",", &save_ptr)) {
> +n_backends++;
> +}
> +
> +free(tokstr);
> +tokstr = xstrdup(node->value);
> +save_ptr = NULL;
> +
> +lb->vips[n_vips].n_backends = n_backends;
> +lb->vips[n_vips].backends = xcalloc(n_backends,
> +sizeof (struct lb_vip_backend));
This should be 'sizeof *lb->vips[n_vips].backends'.
> +
Nit: It's probably fine to remove an empty line here, the one above is already
indented enough to the right.
> +size_t i = 0;
> +for (token = strtok_r(tokstr, ",", &save_ptr);
> +token != NULL;
> +token = strtok_r(NULL, ",", &save_ptr)) {
> +char *backend_ip;
> +uint16_t backend_port;
> +
> +if (!ip_address_and_port_from_key(token, &backend_ip,
> + &backend_port,
> + &addr_family)) {
> +continue;
> +}
> +
> +lb->vips[n_vips].backends[i].ip = backend_ip;
> +lb->vips[n_vips].backends[i].port = backend_port;
> +lb->vips[n_vips].backends[i].addr_family = addr_family;
> +i++;
> +}
> +
> +free(tokstr);
> +n_vips++;
> +}
> +
> +return lb;
> +}
> +
> +struct ovn_lb *
> +ovn_nb_lb_create(const struct nbrec_load_balancer *nbrec_lb,
> + struct hmap *ports, struct hmap *lbs,
> + void * (*ovn_port_find)(const struct hmap *ports,
> + const c
