From 2e2dc4f024312b7351b3d7e964fed42895fd123e Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Fri, 14 Oct 2022 17:57:17 +0200 Subject: [PATCH 1/2] lib,zebra: do not enable link-params when a link-params command fails A given interface has no enabled link-params context. If a link-params configuration command fails, the link-params is wrongly enabled: > r4(config-link-params)# no enable > r4(config-link-params)# delay > (0-16777215) Average delay in micro-second as decimal (0...16777215) > r4(config-link-params)# delay 50 min 300 max 500 > Average delay should be comprise between Min (300) and Max (500) delay > r4(config-link-params)# do sh run zebra > (...) > interface eth-rt1 > link-params > enable > exit-link-params link-params are enabled if and only if the interface structure has a valid link_params pointer. Before checking the command validity, if_link_params_get() is called to retrieve the link-params pointer. However, this function initializes the pointer if it is NULL. Only use if_link_params_get() to retrieve the pointer to avoid confusion. In command setting functions, initialize the link_params pointer if needed only after the validation of the command. Fixes: 16f1b9e ("Update Traffic Engineering Support for OSPFD") Signed-off-by: Louis Scalbert --- lib/if.c | 26 ++++++++++++--- lib/if.h | 2 ++ zebra/interface.c | 83 +++++++++++++++++++++++++++++++++++------------ 3 files changed, 85 insertions(+), 26 deletions(-) diff --git a/lib/if.c b/lib/if.c index fa4fdb82d3..deb0690dcf 100644 --- a/lib/if.c +++ b/lib/if.c @@ -1095,13 +1095,15 @@ const char *if_link_type_str(enum zebra_link_type llt) struct if_link_params *if_link_params_get(struct interface *ifp) { + return ifp->link_params; +} + +struct if_link_params *if_link_params_enable(struct interface *ifp) +{ + struct if_link_params *iflp; int i; - if (ifp->link_params != NULL) - return ifp->link_params; - - struct if_link_params *iflp = - XCALLOC(MTYPE_IF_LINK_PARAMS, sizeof(struct if_link_params)); + iflp = if_link_params_init(ifp); /* Compute default bandwidth based on interface */ iflp->default_bw = @@ -1129,6 +1131,20 @@ struct if_link_params *if_link_params_get(struct interface *ifp) return iflp; } +struct if_link_params *if_link_params_init(struct interface *ifp) +{ + struct if_link_params *iflp = if_link_params_get(ifp); + + if (iflp) + return iflp; + + iflp = XCALLOC(MTYPE_IF_LINK_PARAMS, sizeof(struct if_link_params)); + + ifp->link_params = iflp; + + return iflp; +} + void if_link_params_free(struct interface *ifp) { XFREE(MTYPE_IF_LINK_PARAMS, ifp->link_params); diff --git a/lib/if.h b/lib/if.h index 1c948b875a..478a90d63a 100644 --- a/lib/if.h +++ b/lib/if.h @@ -588,6 +588,8 @@ struct connected *connected_get_linklocal(struct interface *ifp); /* link parameters */ struct if_link_params *if_link_params_get(struct interface *); +struct if_link_params *if_link_params_enable(struct interface *ifp); +struct if_link_params *if_link_params_init(struct interface *ifp); void if_link_params_free(struct interface *); /* Northbound. */ diff --git a/zebra/interface.c b/zebra/interface.c index c674b499ac..5d62ec071f 100644 --- a/zebra/interface.c +++ b/zebra/interface.c @@ -3279,14 +3279,8 @@ DEFUN (link_params_enable, "Link-params: enable TE link parameters on interface %s", ifp->name); - if (!if_link_params_get(ifp)) { - if (IS_ZEBRA_DEBUG_EVENT || IS_ZEBRA_DEBUG_MPLS) - zlog_debug( - "Link-params: failed to init TE link parameters %s", - ifp->name); - - return CMD_WARNING_CONFIG_FAILED; - } + if (!if_link_params_get(ifp)) + if_link_params_enable(ifp); /* force protocols to update LINK STATE due to parameters change */ if (if_is_operative(ifp)) @@ -3330,6 +3324,9 @@ DEFUN (link_params_metric, metric = strtoul(argv[idx_number]->arg, NULL, 10); + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update TE metric if needed */ link_param_cmd_set_uint32(ifp, &iflp->te_metric, LP_TE_METRIC, metric); @@ -3370,17 +3367,20 @@ DEFUN (link_params_maxbw, /* Check that Maximum bandwidth is not lower than other bandwidth * parameters */ - if ((bw <= iflp->max_rsv_bw) || (bw <= iflp->unrsv_bw[0]) - || (bw <= iflp->unrsv_bw[1]) || (bw <= iflp->unrsv_bw[2]) - || (bw <= iflp->unrsv_bw[3]) || (bw <= iflp->unrsv_bw[4]) - || (bw <= iflp->unrsv_bw[5]) || (bw <= iflp->unrsv_bw[6]) - || (bw <= iflp->unrsv_bw[7]) || (bw <= iflp->ava_bw) - || (bw <= iflp->res_bw) || (bw <= iflp->use_bw)) { + if (iflp && ((bw <= iflp->max_rsv_bw) || (bw <= iflp->unrsv_bw[0]) || + (bw <= iflp->unrsv_bw[1]) || (bw <= iflp->unrsv_bw[2]) || + (bw <= iflp->unrsv_bw[3]) || (bw <= iflp->unrsv_bw[4]) || + (bw <= iflp->unrsv_bw[5]) || (bw <= iflp->unrsv_bw[6]) || + (bw <= iflp->unrsv_bw[7]) || (bw <= iflp->ava_bw) || + (bw <= iflp->res_bw) || (bw <= iflp->use_bw))) { vty_out(vty, "Maximum Bandwidth could not be lower than others bandwidth\n"); return CMD_WARNING_CONFIG_FAILED; } + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update Maximum Bandwidth if needed */ link_param_cmd_set_float(ifp, &iflp->max_bw, LP_MAX_BW, bw); @@ -3406,13 +3406,16 @@ DEFUN (link_params_max_rsv_bw, /* Check that bandwidth is not greater than maximum bandwidth parameter */ - if (bw > iflp->max_bw) { + if (iflp && bw > iflp->max_bw) { vty_out(vty, "Maximum Reservable Bandwidth could not be greater than Maximum Bandwidth (%g)\n", iflp->max_bw); return CMD_WARNING_CONFIG_FAILED; } + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update Maximum Reservable Bandwidth if needed */ link_param_cmd_set_float(ifp, &iflp->max_rsv_bw, LP_MAX_RSV_BW, bw); @@ -3448,13 +3451,16 @@ DEFUN (link_params_unrsv_bw, /* Check that bandwidth is not greater than maximum bandwidth parameter */ - if (bw > iflp->max_bw) { + if (iflp && bw > iflp->max_bw) { vty_out(vty, "UnReserved Bandwidth could not be greater than Maximum Bandwidth (%g)\n", iflp->max_bw); return CMD_WARNING_CONFIG_FAILED; } + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update Unreserved Bandwidth if needed */ link_param_cmd_set_float(ifp, &iflp->unrsv_bw[priority], LP_UNRSV_BW, bw); @@ -3479,6 +3485,9 @@ DEFUN (link_params_admin_grp, return CMD_WARNING_CONFIG_FAILED; } + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update Administrative Group if needed */ link_param_cmd_set_uint32(ifp, &iflp->admin_grp, LP_ADM_GRP, value); @@ -3521,6 +3530,9 @@ DEFUN (link_params_inter_as, return CMD_WARNING_CONFIG_FAILED; } + if (!iflp) + iflp = if_link_params_enable(ifp); + as = strtoul(argv[idx_number]->arg, NULL, 10); /* Update Remote IP and Remote AS fields if needed */ @@ -3548,6 +3560,9 @@ DEFUN (no_link_params_inter_as, VTY_DECLVAR_CONTEXT(interface, ifp); struct if_link_params *iflp = if_link_params_get(ifp); + if (!iflp) + return CMD_SUCCESS; + /* Reset Remote IP and AS neighbor */ iflp->rmt_as = 0; iflp->rmt_ip.s_addr = 0; @@ -3595,13 +3610,17 @@ DEFUN (link_params_delay, * Therefore, it is also allowed that the average * delay be equal to the min delay or max delay. */ - if (IS_PARAM_SET(iflp, LP_MM_DELAY) - && (delay < iflp->min_delay || delay > iflp->max_delay)) { + if (iflp && IS_PARAM_SET(iflp, LP_MM_DELAY) && + (delay < iflp->min_delay || delay > iflp->max_delay)) { vty_out(vty, "Average delay should be in range Min (%d) - Max (%d) delay\n", iflp->min_delay, iflp->max_delay); return CMD_WARNING_CONFIG_FAILED; } + + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update delay if value is not set or change */ if (IS_PARAM_UNSET(iflp, LP_DELAY) || iflp->av_delay != delay) { iflp->av_delay = delay; @@ -3626,6 +3645,10 @@ DEFUN (link_params_delay, low, high); return CMD_WARNING_CONFIG_FAILED; } + + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update Delays if needed */ if (IS_PARAM_UNSET(iflp, LP_DELAY) || IS_PARAM_UNSET(iflp, LP_MM_DELAY) @@ -3656,6 +3679,9 @@ DEFUN (no_link_params_delay, VTY_DECLVAR_CONTEXT(interface, ifp); struct if_link_params *iflp = if_link_params_get(ifp); + if (!iflp) + return CMD_SUCCESS; + /* Unset Delays */ iflp->av_delay = 0; UNSET_PARAM(iflp, LP_DELAY); @@ -3683,6 +3709,9 @@ DEFUN (link_params_delay_var, value = strtoul(argv[idx_number]->arg, NULL, 10); + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update Delay Variation if needed */ link_param_cmd_set_uint32(ifp, &iflp->delay_var, LP_DELAY_VAR, value); @@ -3723,6 +3752,9 @@ DEFUN (link_params_pkt_loss, if (fval > MAX_PKT_LOSS) fval = MAX_PKT_LOSS; + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update Packet Loss if needed */ link_param_cmd_set_float(ifp, &iflp->pkt_loss, LP_PKT_LOSS, fval); @@ -3762,13 +3794,16 @@ DEFUN (link_params_res_bw, /* Check that bandwidth is not greater than maximum bandwidth parameter */ - if (bw > iflp->max_bw) { + if (iflp && bw > iflp->max_bw) { vty_out(vty, "Residual Bandwidth could not be greater than Maximum Bandwidth (%g)\n", iflp->max_bw); return CMD_WARNING_CONFIG_FAILED; } + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update Residual Bandwidth if needed */ link_param_cmd_set_float(ifp, &iflp->res_bw, LP_RES_BW, bw); @@ -3808,13 +3843,16 @@ DEFUN (link_params_ava_bw, /* Check that bandwidth is not greater than maximum bandwidth parameter */ - if (bw > iflp->max_bw) { + if (iflp && bw > iflp->max_bw) { vty_out(vty, "Available Bandwidth could not be greater than Maximum Bandwidth (%g)\n", iflp->max_bw); return CMD_WARNING_CONFIG_FAILED; } + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update Residual Bandwidth if needed */ link_param_cmd_set_float(ifp, &iflp->ava_bw, LP_AVA_BW, bw); @@ -3854,13 +3892,16 @@ DEFUN (link_params_use_bw, /* Check that bandwidth is not greater than maximum bandwidth parameter */ - if (bw > iflp->max_bw) { + if (iflp && bw > iflp->max_bw) { vty_out(vty, "Utilised Bandwidth could not be greater than Maximum Bandwidth (%g)\n", iflp->max_bw); return CMD_WARNING_CONFIG_FAILED; } + if (!iflp) + iflp = if_link_params_enable(ifp); + /* Update Utilized Bandwidth if needed */ link_param_cmd_set_float(ifp, &iflp->use_bw, LP_USE_BW, bw); From fe0a129687c530d95377a6ed7c14d578c5be5996 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Fri, 14 Oct 2022 17:57:20 +0200 Subject: [PATCH 2/2] lib,zebra: link-params are not flushed after no enable Daemons like isisd continue to use the previous link-params after they are removed from zebra. For example, >r0# sh run zebra > (...) > interface eth-rt1 > link-params > enable > metric 100 > exit-link-params > r0# conf > r0(config)# interface eth-rt1 > r0(config-if)# link-params > r0(config-link-params)# no enable After "no enable", "sh run zebra" displays no more link-params context. The "no enable" causes the release of the "link_params" pointer within the "interface" structure. The zebra function to update daemons with a ZEBRA_INTERFACE_LINK_PARAMS zapi message is called but the function returns without doing anything because the "link_params" pointer is NULL. Therefore, the "link_params" pointers are kept in daemons. When the zebra "link_params" pointer is NULL: - Send a zapi link param message that contains no link parameters instead of sending no message. - At reception in daemons, the absence of link parameters causes the release of the "link_params" pointer. Fixes: 16f1b9e ("Update Traffic Engineering Support for OSPFD") Signed-off-by: Louis Scalbert --- lib/zclient.c | 69 +++++++++++++++++++++++++++++++++--------------- zebra/zapi_msg.c | 5 ---- 2 files changed, 47 insertions(+), 27 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index 8ec82ab7bb..f5d45b40ef 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -2299,13 +2299,22 @@ static int zclient_handle_error(ZAPI_CALLBACK_ARGS) return 0; } -static int link_params_set_value(struct stream *s, struct if_link_params *iflp) +static int link_params_set_value(struct stream *s, struct interface *ifp) { + uint8_t link_params_enabled; + struct if_link_params *iflp; + uint32_t bwclassnum; + + iflp = if_link_params_get(ifp); if (iflp == NULL) - return -1; + iflp = if_link_params_init(ifp); - uint32_t bwclassnum; + STREAM_GETC(s, link_params_enabled); + if (!link_params_enabled) { + if_link_params_free(ifp); + return 0; + } STREAM_GETL(s, iflp->lp_status); STREAM_GETL(s, iflp->te_metric); @@ -2346,9 +2355,9 @@ struct interface *zebra_interface_link_params_read(struct stream *s, bool *changed) { struct if_link_params *iflp; - struct if_link_params iflp_copy; + struct if_link_params iflp_prev; ifindex_t ifindex; - bool params_changed = false; + bool iflp_prev_set; STREAM_GETL(s, ifindex); @@ -2361,22 +2370,32 @@ struct interface *zebra_interface_link_params_read(struct stream *s, return NULL; } - if (ifp->link_params == NULL) - params_changed = true; + iflp = if_link_params_get(ifp); + if (iflp) { + iflp_prev_set = true; + memcpy(&iflp_prev, ifp->link_params, sizeof(iflp_prev)); + } else + iflp_prev_set = false; - if ((iflp = if_link_params_get(ifp)) == NULL) - return NULL; - - memcpy(&iflp_copy, iflp, sizeof(iflp_copy)); - - if (link_params_set_value(s, iflp) != 0) + /* read the link_params from stream + * Free ifp->link_params if the stream has no params + * to means that link-params are not enabled on links. + */ + if (link_params_set_value(s, ifp) != 0) goto stream_failure; - if (memcmp(&iflp_copy, iflp, sizeof(iflp_copy))) - params_changed = true; + if (changed == NULL) + return ifp; - if (changed) - *changed = params_changed; + if (iflp_prev_set && iflp) { + if (memcmp(&iflp_prev, iflp, sizeof(iflp_prev))) + *changed = true; + else + *changed = false; + } else if (!iflp_prev_set && !iflp) + *changed = false; + else + *changed = true; return ifp; @@ -2415,10 +2434,8 @@ static void zebra_interface_if_set_value(struct stream *s, /* Read Traffic Engineering status */ link_params_status = stream_getc(s); /* Then, Traffic Engineering parameters if any */ - if (link_params_status) { - struct if_link_params *iflp = if_link_params_get(ifp); - link_params_set_value(s, iflp); - } + if (link_params_status) + link_params_set_value(s, ifp); nexthop_group_interface_state_change(ifp, old_ifindex); @@ -2435,12 +2452,20 @@ size_t zebra_interface_link_params_write(struct stream *s, struct if_link_params *iflp; int i; - if (s == NULL || ifp == NULL || ifp->link_params == NULL) + if (s == NULL || ifp == NULL) return 0; iflp = ifp->link_params; w = 0; + /* encode if link_params is enabled */ + if (iflp) { + w += stream_putc(s, true); + } else { + w += stream_putc(s, false); + return w; + } + w += stream_putl(s, iflp->lp_status); w += stream_putl(s, iflp->te_metric); diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 761ba789b8..4d7ad21bf3 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -232,11 +232,6 @@ int zsend_interface_link_params(struct zserv *client, struct interface *ifp) { struct stream *s = stream_new(ZEBRA_MAX_PACKET_SIZ); - if (!ifp->link_params) { - stream_free(s); - return 0; - } - zclient_create_header(s, ZEBRA_INTERFACE_LINK_PARAMS, ifp->vrf->vrf_id); /* Add Interface Index */