[ovs-dev] [PATCH ovn v5 1/6] Spin out flow generation into build_dhcpv4_options_flows

Dumitru Ceara dceara at redhat.com
Tue May 12 13:40:39 UTC 2020


On 5/11/20 7:00 PM, Ihar Hrachyshka wrote:
> Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>
> ---
>  northd/ovn-northd.c | 185 ++++++++++++++++++++++++--------------------
>  1 file changed, 99 insertions(+), 86 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 742aad85e..884f85701 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -6075,6 +6075,98 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
>      sset_destroy(&all_ips_v6);
>  }
>  
> +
> +static void
> +build_dhcpv4_options_flows(struct ovn_port *op,
> +                           struct lport_addresses *lsp_addrs,
> +                           const char *json_key, bool is_external,
> +                           struct hmap *lflows)
> +{
> +    struct ds match = DS_EMPTY_INITIALIZER;
> +
> +    struct ovsdb_idl_row *stage_hint;
> +    if (op->nbsp->dhcpv4_options) {
> +        stage_hint = &op->nbsp->dhcpv4_options->header_;
> +    } else {
> +        stage_hint = NULL;
> +    }
> +
> +    for (size_t j = 0; j < lsp_addrs->n_ipv4_addrs; j++) {
> +        struct ds options_action = DS_EMPTY_INITIALIZER;
> +        struct ds response_action = DS_EMPTY_INITIALIZER;
> +        struct ds ipv4_addr_match = DS_EMPTY_INITIALIZER;
> +        if (build_dhcpv4_action(
> +                op, lsp_addrs->ipv4_addrs[j].addr,
> +                &options_action, &response_action, &ipv4_addr_match)) {
> +            ds_clear(&match);
> +            ds_put_format(
> +                &match, "inport == %s && eth.src == %s && "
> +                "ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && "
> +                "udp.src == 68 && udp.dst == 67",
> +                json_key, lsp_addrs->ea_s);
> +
> +            if (is_external) {
> +                ds_put_format(&match, " && is_chassis_resident(%s)",
> +                              op->json_key);
> +            }
> +
> +            ovn_lflow_add_with_hint(lflows, op->od,
> +                                    S_SWITCH_IN_DHCP_OPTIONS, 100,
> +                                    ds_cstr(&match),
> +                                    ds_cstr(&options_action),
> +                                    stage_hint);
> +            ds_clear(&match);
> +            /* Allow ip4.src = OFFER_IP and
> +             * ip4.dst = {SERVER_IP, 255.255.255.255} for the below
> +             * cases
> +             *  -  When the client wants to renew the IP by sending
> +             *     the DHCPREQUEST to the server ip.
> +             *  -  When the client wants to renew the IP by
> +             *     broadcasting the DHCPREQUEST.
> +             */
> +            ds_put_format(
> +                &match, "inport == %s && eth.src == %s && "
> +                "%s && udp.src == 68 && udp.dst == 67",
> +                json_key, lsp_addrs->ea_s, ds_cstr(&ipv4_addr_match));
> +
> +            if (is_external) {
> +                ds_put_format(&match, " && is_chassis_resident(%s)",
> +                              op->json_key);
> +            }
> +
> +            ovn_lflow_add_with_hint(lflows, op->od,
> +                                    S_SWITCH_IN_DHCP_OPTIONS, 100,
> +                                    ds_cstr(&match),
> +                                    ds_cstr(&options_action),
> +                                    stage_hint);
> +            ds_clear(&match);
> +
> +            /* If REGBIT_DHCP_OPTS_RESULT is set, it means the
> +             * put_dhcp_opts action is successful. */
> +            ds_put_format(
> +                &match, "inport == %s && eth.src == %s && "
> +                "ip4 && udp.src == 68 && udp.dst == 67"
> +                " && "REGBIT_DHCP_OPTS_RESULT,
> +                json_key, lsp_addrs->ea_s);
> +
> +            if (is_external) {
> +                ds_put_format(&match, " && is_chassis_resident(%s)",
> +                              op->json_key);
> +            }
> +
> +            ovn_lflow_add_with_hint(lflows, op->od,
> +                                    S_SWITCH_IN_DHCP_RESPONSE, 100,
> +                                    ds_cstr(&match),
> +                                    ds_cstr(&response_action),
> +                                    stage_hint);
> +            ds_destroy(&options_action);
> +            ds_destroy(&response_action);
> +            ds_destroy(&ipv4_addr_match);
> +            break;
> +        }
> +    }

Hi Ihar,

We miss a ds_destroy(&match) here without which we might leak memory.

One way to test for it is:
make check-valgrind TESTSUITEFLAGS="45"

Valgrind results are stored in:
tests/testsuite.dir/045/valgrind*

Thanks,
Dumitru

> +}
> +
>  static void
>  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>                      struct hmap *port_groups, struct hmap *lflows,
> @@ -6396,95 +6488,16 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>          }
>  
>          for (size_t i = 0; i < op->n_lsp_addrs; i++) {
> -            struct ovsdb_idl_row *stage_hint;
> -
> -            if (op->nbsp->dhcpv4_options) {
> -                stage_hint = &op->nbsp->dhcpv4_options->header_;
> +            const char *json_key;
> +            if (is_external) {
> +                json_key = op->od->localnet_port->json_key;
>              } else {
> -                stage_hint = NULL;
> -            }
> -
> -            for (size_t j = 0; j < op->lsp_addrs[i].n_ipv4_addrs; j++) {
> -                struct ds options_action = DS_EMPTY_INITIALIZER;
> -                struct ds response_action = DS_EMPTY_INITIALIZER;
> -                struct ds ipv4_addr_match = DS_EMPTY_INITIALIZER;
> -                if (build_dhcpv4_action(
> -                        op, op->lsp_addrs[i].ipv4_addrs[j].addr,
> -                        &options_action, &response_action, &ipv4_addr_match)) {
> -                    ds_clear(&match);
> -                    ds_put_format(
> -                        &match, "inport == %s && eth.src == %s && "
> -                        "ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && "
> -                        "udp.src == 68 && udp.dst == 67",
> -                        is_external ? op->od->localnet_port->json_key :
> -                            op->json_key,
> -                        op->lsp_addrs[i].ea_s);
> -
> -                    if (is_external) {
> -                        ds_put_format(&match, " && is_chassis_resident(%s)",
> -                                      op->json_key);
> -                    }
> -
> -                    ovn_lflow_add_with_hint(lflows, op->od,
> -                                            S_SWITCH_IN_DHCP_OPTIONS, 100,
> -                                            ds_cstr(&match),
> -                                            ds_cstr(&options_action),
> -                                            stage_hint);
> -                    ds_clear(&match);
> -                    /* Allow ip4.src = OFFER_IP and
> -                     * ip4.dst = {SERVER_IP, 255.255.255.255} for the below
> -                     * cases
> -                     *  -  When the client wants to renew the IP by sending
> -                     *     the DHCPREQUEST to the server ip.
> -                     *  -  When the client wants to renew the IP by
> -                     *     broadcasting the DHCPREQUEST.
> -                     */
> -                    ds_put_format(
> -                        &match, "inport == %s && eth.src == %s && "
> -                        "%s && udp.src == 68 && udp.dst == 67",
> -                        is_external ? op->od->localnet_port->json_key :
> -                            op->json_key,
> -                        op->lsp_addrs[i].ea_s, ds_cstr(&ipv4_addr_match));
> -
> -                    if (is_external) {
> -                        ds_put_format(&match, " && is_chassis_resident(%s)",
> -                                      op->json_key);
> -                    }
> -
> -                    ovn_lflow_add_with_hint(lflows, op->od,
> -                                            S_SWITCH_IN_DHCP_OPTIONS, 100,
> -                                            ds_cstr(&match),
> -                                            ds_cstr(&options_action),
> -                                            stage_hint);
> -                    ds_clear(&match);
> -
> -                    /* If REGBIT_DHCP_OPTS_RESULT is set, it means the
> -                     * put_dhcp_opts action  is successful. */
> -                    ds_put_format(
> -                        &match, "inport == %s && eth.src == %s && "
> -                        "ip4 && udp.src == 68 && udp.dst == 67"
> -                        " && "REGBIT_DHCP_OPTS_RESULT,
> -                        is_external ? op->od->localnet_port->json_key :
> -                            op->json_key,
> -                        op->lsp_addrs[i].ea_s);
> -
> -                    if (is_external) {
> -                        ds_put_format(&match, " && is_chassis_resident(%s)",
> -                                      op->json_key);
> -                    }
> -
> -                    ovn_lflow_add_with_hint(lflows, op->od,
> -                                            S_SWITCH_IN_DHCP_RESPONSE, 100,
> -                                            ds_cstr(&match),
> -                                            ds_cstr(&response_action),
> -                                            stage_hint);
> -                    ds_destroy(&options_action);
> -                    ds_destroy(&response_action);
> -                    ds_destroy(&ipv4_addr_match);
> -                    break;
> -                }
> +                json_key = op->json_key;
>              }
> +            build_dhcpv4_options_flows(op, &op->lsp_addrs[i], json_key,
> +                                       is_external, lflows);
>  
> +            struct ovsdb_idl_row *stage_hint;
>              if (op->nbsp->dhcpv6_options) {
>                  stage_hint = &op->nbsp->dhcpv6_options->header_;
>              } else {
> 



More information about the dev mailing list