| Message ID | 1582818859-32411-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn,v2] ovn-northd: Fix IGMP_Group port extraction. | expand |
Acked-by: Mark Michelson <mmichels@redhat.com> On 2/27/20 10:54 AM, Dumitru Ceara wrote: > It can happen that all ports on which IGMP/MLD join reports were > received for a multicast group are already configured to flood multicast > traffic. In such cases we can safely skip the IGMP_Group record > completely. Otherwise we risk trying to install an entry in the > Southbound DB Multicast_Group table with 0 ports, triggering a > transaction error. > > Also, remove the duplicated call to ovn_port_find() in function > ovn_igmp_group_get_ports() and add a check for NULL port. > > Reported-by: Ying Xu <yinxu@redhat.com> > Reported-at: https://bugzilla.redhat.com/1805652 > Fixes: 79308138891a ("ovn-northd: Add static IP multicast flood configuration") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > V2: Remove duplicate call to ovn_port_find(). > --- > northd/ovn-northd.c | 34 +++++++++++++++++++++++----------- > 1 file changed, 23 insertions(+), 11 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index d007ba6..9abbec9 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -3692,13 +3692,17 @@ static struct ovn_port ** > ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, > size_t *n_ports, struct hmap *ovn_ports) > { > - struct ovn_port **ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); > + struct ovn_port **ports = NULL; > > *n_ports = 0; > for (size_t i = 0; i < sb_igmp_group->n_ports; i++) { > struct ovn_port *port = > ovn_port_find(ovn_ports, sb_igmp_group->ports[i]->logical_port); > > + if (!port) { > + continue; > + } > + > /* If this is already a flood port skip it for the group. */ > if (port->mcast_info.flood) { > continue; > @@ -3712,11 +3716,12 @@ ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, > continue; > } > > - ports[(*n_ports)] = port; > - ovn_port_find(ovn_ports, sb_igmp_group->ports[i]->logical_port); > - if (ports[(*n_ports)]) { > - (*n_ports)++; > + if (ports == NULL) { > + ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); > } > + > + ports[(*n_ports)] = port; > + (*n_ports)++; > } > > return ports; > @@ -10617,6 +10622,18 @@ build_mcast_groups(struct northd_context *ctx, > continue; > } > > + /* Extract the IGMP group ports from the SB entry. */ > + size_t n_igmp_ports; > + struct ovn_port **igmp_ports = > + ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); > + > + /* It can be that all ports in the IGMP group record already have > + * mcast_flood=true and then we can skip the group completely. > + */ > + if (!igmp_ports) { > + continue; > + } > + > /* Add the IGMP group entry. Will also try to allocate an ID for it > * if the multicast group already exists. > */ > @@ -10624,12 +10641,7 @@ build_mcast_groups(struct northd_context *ctx, > ovn_igmp_group_add(ctx, igmp_groups, od, &group_address, > sb_igmp->address); > > - /* Extract the IGMP group ports from the SB entry and store them > - * in the IGMP group. > - */ > - size_t n_igmp_ports; > - struct ovn_port **igmp_ports = > - ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); > + /* Add the extracted ports to the IGMP group. */ > ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports); > } > >
On Fri, Feb 28, 2020 at 2:08 AM Mark Michelson <mmichels@redhat.com> wrote: > > Acked-by: Mark Michelson <mmichels@redhat.com> Thanks Dumitru and Mark. I applied this patch to master and branch-20.03 Thanks Numan > > On 2/27/20 10:54 AM, Dumitru Ceara wrote: > > It can happen that all ports on which IGMP/MLD join reports were > > received for a multicast group are already configured to flood multicast > > traffic. In such cases we can safely skip the IGMP_Group record > > completely. Otherwise we risk trying to install an entry in the > > Southbound DB Multicast_Group table with 0 ports, triggering a > > transaction error. > > > > Also, remove the duplicated call to ovn_port_find() in function > > ovn_igmp_group_get_ports() and add a check for NULL port. > > > > Reported-by: Ying Xu <yinxu@redhat.com> > > Reported-at: https://bugzilla.redhat.com/1805652 > > Fixes: 79308138891a ("ovn-northd: Add static IP multicast flood configuration") > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > > > --- > > V2: Remove duplicate call to ovn_port_find(). > > --- > > northd/ovn-northd.c | 34 +++++++++++++++++++++++----------- > > 1 file changed, 23 insertions(+), 11 deletions(-) > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index d007ba6..9abbec9 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -3692,13 +3692,17 @@ static struct ovn_port ** > > ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, > > size_t *n_ports, struct hmap *ovn_ports) > > { > > - struct ovn_port **ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); > > + struct ovn_port **ports = NULL; > > > > *n_ports = 0; > > for (size_t i = 0; i < sb_igmp_group->n_ports; i++) { > > struct ovn_port *port = > > ovn_port_find(ovn_ports, sb_igmp_group->ports[i]->logical_port); > > > > + if (!port) { > > + continue; > > + } > > + > > /* If this is already a flood port skip it for the group. */ > > if (port->mcast_info.flood) { > > continue; > > @@ -3712,11 +3716,12 @@ ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, > > continue; > > } > > > > - ports[(*n_ports)] = port; > > - ovn_port_find(ovn_ports, sb_igmp_group->ports[i]->logical_port); > > - if (ports[(*n_ports)]) { > > - (*n_ports)++; > > + if (ports == NULL) { > > + ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); > > } > > + > > + ports[(*n_ports)] = port; > > + (*n_ports)++; > > } > > > > return ports; > > @@ -10617,6 +10622,18 @@ build_mcast_groups(struct northd_context *ctx, > > continue; > > } > > > > + /* Extract the IGMP group ports from the SB entry. */ > > + size_t n_igmp_ports; > > + struct ovn_port **igmp_ports = > > + ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); > > + > > + /* It can be that all ports in the IGMP group record already have > > + * mcast_flood=true and then we can skip the group completely. > > + */ > > + if (!igmp_ports) { > > + continue; > > + } > > + > > /* Add the IGMP group entry. Will also try to allocate an ID for it > > * if the multicast group already exists. > > */ > > @@ -10624,12 +10641,7 @@ build_mcast_groups(struct northd_context *ctx, > > ovn_igmp_group_add(ctx, igmp_groups, od, &group_address, > > sb_igmp->address); > > > > - /* Extract the IGMP group ports from the SB entry and store them > > - * in the IGMP group. > > - */ > > - size_t n_igmp_ports; > > - struct ovn_port **igmp_ports = > > - ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); > > + /* Add the extracted ports to the IGMP group. */ > > ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports); > > } > > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index d007ba6..9abbec9 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -3692,13 +3692,17 @@ static struct ovn_port ** ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, size_t *n_ports, struct hmap *ovn_ports) { - struct ovn_port **ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); + struct ovn_port **ports = NULL; *n_ports = 0; for (size_t i = 0; i < sb_igmp_group->n_ports; i++) { struct ovn_port *port = ovn_port_find(ovn_ports, sb_igmp_group->ports[i]->logical_port); + if (!port) { + continue; + } + /* If this is already a flood port skip it for the group. */ if (port->mcast_info.flood) { continue; @@ -3712,11 +3716,12 @@ ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, continue; } - ports[(*n_ports)] = port; - ovn_port_find(ovn_ports, sb_igmp_group->ports[i]->logical_port); - if (ports[(*n_ports)]) { - (*n_ports)++; + if (ports == NULL) { + ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); } + + ports[(*n_ports)] = port; + (*n_ports)++; } return ports; @@ -10617,6 +10622,18 @@ build_mcast_groups(struct northd_context *ctx, continue; } + /* Extract the IGMP group ports from the SB entry. */ + size_t n_igmp_ports; + struct ovn_port **igmp_ports = + ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); + + /* It can be that all ports in the IGMP group record already have + * mcast_flood=true and then we can skip the group completely. + */ + if (!igmp_ports) { + continue; + } + /* Add the IGMP group entry. Will also try to allocate an ID for it * if the multicast group already exists. */ @@ -10624,12 +10641,7 @@ build_mcast_groups(struct northd_context *ctx, ovn_igmp_group_add(ctx, igmp_groups, od, &group_address, sb_igmp->address); - /* Extract the IGMP group ports from the SB entry and store them - * in the IGMP group. - */ - size_t n_igmp_ports; - struct ovn_port **igmp_ports = - ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); + /* Add the extracted ports to the IGMP group. */ ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports); }
It can happen that all ports on which IGMP/MLD join reports were received for a multicast group are already configured to flood multicast traffic. In such cases we can safely skip the IGMP_Group record completely. Otherwise we risk trying to install an entry in the Southbound DB Multicast_Group table with 0 ports, triggering a transaction error. Also, remove the duplicated call to ovn_port_find() in function ovn_igmp_group_get_ports() and add a check for NULL port. Reported-by: Ying Xu <yinxu@redhat.com> Reported-at: https://bugzilla.redhat.com/1805652 Fixes: 79308138891a ("ovn-northd: Add static IP multicast flood configuration") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- V2: Remove duplicate call to ovn_port_find(). --- northd/ovn-northd.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-)