From 8249f96a5f667af016764219f501d5015104061d Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 28 Oct 2021 09:15:44 -0400 Subject: [PATCH 1/3] zebra: dplane_ctx_get_pbr_ipset should return void The function call dplane_ctx_get_pbr_ipset only returns false when the calling function fails to pass in a valid ipset pointer. This should be an assertion issue since it's a programming issue as opposed to an actual run time issue. Change the function call parameter to not return a bool on success/fail for a compile time decision. Signed-off-by: Donald Sharp --- zebra/zapi_msg.c | 6 ++---- zebra/zebra_dplane.c | 15 +++++++-------- zebra/zebra_dplane.h | 5 ++--- zebra/zebra_pbr.c | 18 +++++++++--------- 4 files changed, 20 insertions(+), 24 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 85721d7d70..7cf248d08c 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -906,8 +906,7 @@ void zsend_ipset_notify_owner(const struct zebra_dplane_ctx *ctx, struct zebra_pbr_ipset ipset; uint16_t cmd = ZEBRA_IPSET_NOTIFY_OWNER; - if (!dplane_ctx_get_pbr_ipset(ctx, &ipset)) - return; + dplane_ctx_get_pbr_ipset(ctx, &ipset); if (IS_ZEBRA_DEBUG_PACKET) zlog_debug("%s: Notifying %s id %u note %u", __func__, @@ -944,8 +943,7 @@ void zsend_ipset_entry_notify_owner(const struct zebra_dplane_ctx *ctx, if (!dplane_ctx_get_pbr_ipset_entry(ctx, &ipent)) return; - if (!dplane_ctx_get_pbr_ipset(ctx, &ipset)) - return; + dplane_ctx_get_pbr_ipset(ctx, &ipset); if (IS_ZEBRA_DEBUG_PACKET) zlog_debug("%s: Notifying %s id %u note %u", __func__, diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index 3d258e0829..9c17ab6892 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -2155,13 +2155,13 @@ dplane_ctx_get_pbr_iptable(const struct zebra_dplane_ctx *ctx, return true; } -bool dplane_ctx_get_pbr_ipset(const struct zebra_dplane_ctx *ctx, +void dplane_ctx_get_pbr_ipset(const struct zebra_dplane_ctx *ctx, struct zebra_pbr_ipset *ipset) { DPLANE_CTX_VALID(ctx); - if (!ipset) - return false; + assert(ipset); + if (ctx->zd_op == DPLANE_OP_IPSET_ENTRY_ADD || ctx->zd_op == DPLANE_OP_IPSET_ENTRY_DELETE) { memset(ipset, 0, sizeof(struct zebra_pbr_ipset)); @@ -2171,7 +2171,6 @@ bool dplane_ctx_get_pbr_ipset(const struct zebra_dplane_ctx *ctx, ZEBRA_IPSET_NAME_SIZE); } else memcpy(ipset, &ctx->u.ipset, sizeof(struct zebra_pbr_ipset)); - return true; } bool dplane_ctx_get_pbr_ipset_entry(const struct zebra_dplane_ctx *ctx, @@ -5068,10 +5067,10 @@ static void kernel_dplane_log_detail(struct zebra_dplane_ctx *ctx) case DPLANE_OP_IPSET_DELETE: { struct zebra_pbr_ipset ipset; - if (dplane_ctx_get_pbr_ipset(ctx, &ipset)) - zlog_debug("Dplane ipset update op %s, unique(%u), ctx %p", - dplane_op2str(dplane_ctx_get_op(ctx)), - ipset.unique, ctx); + dplane_ctx_get_pbr_ipset(ctx, &ipset); + zlog_debug("Dplane ipset update op %s, unique(%u), ctx %p", + dplane_op2str(dplane_ctx_get_op(ctx)), ipset.unique, + ctx); } break; case DPLANE_OP_IPSET_ENTRY_ADD: case DPLANE_OP_IPSET_ENTRY_DELETE: { diff --git a/zebra/zebra_dplane.h b/zebra/zebra_dplane.h index a23de61c80..d3a232c9da 100644 --- a/zebra/zebra_dplane.h +++ b/zebra/zebra_dplane.h @@ -530,9 +530,8 @@ bool dplane_ctx_get_pbr_iptable(const struct zebra_dplane_ctx *ctx, struct zebra_pbr_iptable *table); struct zebra_pbr_ipset; -bool -dplane_ctx_get_pbr_ipset(const struct zebra_dplane_ctx *ctx, - struct zebra_pbr_ipset *ipset); +void dplane_ctx_get_pbr_ipset(const struct zebra_dplane_ctx *ctx, + struct zebra_pbr_ipset *ipset); struct zebra_pbr_ipset_entry; bool dplane_ctx_get_pbr_ipset_entry(const struct zebra_dplane_ctx *ctx, diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c index 3607110aa2..b0d274e7c1 100644 --- a/zebra/zebra_pbr.c +++ b/zebra/zebra_pbr.c @@ -571,13 +571,13 @@ void zebra_pbr_process_ipset(struct zebra_dplane_ctx *ctx) mode = 1; else mode = 0; - if (dplane_ctx_get_pbr_ipset(ctx, &ipset)) { - ret = hook_call(zebra_pbr_ipset_update, mode, &ipset); - if (ret) - dplane_ctx_set_status(ctx, - ZEBRA_DPLANE_REQUEST_SUCCESS); - } - if (!ret) + + dplane_ctx_get_pbr_ipset(ctx, &ipset); + + ret = hook_call(zebra_pbr_ipset_update, mode, &ipset); + if (ret) + dplane_ctx_set_status(ctx, ZEBRA_DPLANE_REQUEST_SUCCESS); + else dplane_ctx_set_status(ctx, ZEBRA_DPLANE_REQUEST_FAILURE); } @@ -594,8 +594,8 @@ void zebra_pbr_process_ipset_entry(struct zebra_dplane_ctx *ctx) if (!dplane_ctx_get_pbr_ipset_entry(ctx, &ipset_entry)) return; - if (!dplane_ctx_get_pbr_ipset(ctx, &ipset)) - return; + dplane_ctx_get_pbr_ipset(ctx, &ipset); + ipset_entry.backpointer = &ipset; ret = hook_call(zebra_pbr_ipset_entry_update, mode, &ipset_entry); From 8d78e148b852ee31399d9a66af948cf604016ebe Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 28 Oct 2021 10:35:51 -0400 Subject: [PATCH 2/3] zebra: return void for dplane_ctx_get_pbr_iptable The only time this function ever failed is when the developer does not pass in a usable pointer to place the data in. Change it to an assert to signify to the end developer that is what we want and then remove all the if checks for failure Signed-off-by: Donald Sharp --- zebra/zapi_msg.c | 3 +-- zebra/zebra_dplane.c | 13 ++++++------- zebra/zebra_dplane.h | 5 ++--- zebra/zebra_pbr.c | 13 ++++++------- 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 7cf248d08c..5bacf1f40a 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -871,8 +871,7 @@ void zsend_iptable_notify_owner(const struct zebra_dplane_ctx *ctx, struct zebra_pbr_iptable ipt; uint16_t cmd = ZEBRA_IPTABLE_NOTIFY_OWNER; - if (!dplane_ctx_get_pbr_iptable(ctx, &ipt)) - return; + dplane_ctx_get_pbr_iptable(ctx, &ipt); if (IS_ZEBRA_DEBUG_PACKET) zlog_debug("%s: Notifying %s id %u note %u", __func__, diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index 9c17ab6892..d02d0178cc 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -2145,14 +2145,12 @@ dplane_ctx_get_br_port_backup_nhg_id(const struct zebra_dplane_ctx *ctx) } /* Accessors for PBR iptable information */ -bool -dplane_ctx_get_pbr_iptable(const struct zebra_dplane_ctx *ctx, - struct zebra_pbr_iptable *table) +void dplane_ctx_get_pbr_iptable(const struct zebra_dplane_ctx *ctx, + struct zebra_pbr_iptable *table) { DPLANE_CTX_VALID(ctx); memcpy(table, &ctx->u.iptable, sizeof(struct zebra_pbr_iptable)); - return true; } void dplane_ctx_get_pbr_ipset(const struct zebra_dplane_ctx *ctx, @@ -5059,9 +5057,10 @@ static void kernel_dplane_log_detail(struct zebra_dplane_ctx *ctx) case DPLANE_OP_IPTABLE_DELETE: { struct zebra_pbr_iptable ipt; - if (dplane_ctx_get_pbr_iptable(ctx, &ipt)) - zlog_debug("Dplane iptable update op %s, unique(%u), ctx %p", - dplane_op2str(dplane_ctx_get_op(ctx)), ipt.unique, ctx); + dplane_ctx_get_pbr_iptable(ctx, &ipt); + zlog_debug("Dplane iptable update op %s, unique(%u), ctx %p", + dplane_op2str(dplane_ctx_get_op(ctx)), ipt.unique, + ctx); } break; case DPLANE_OP_IPSET_ADD: case DPLANE_OP_IPSET_DELETE: { diff --git a/zebra/zebra_dplane.h b/zebra/zebra_dplane.h index d3a232c9da..ea408116ee 100644 --- a/zebra/zebra_dplane.h +++ b/zebra/zebra_dplane.h @@ -526,9 +526,8 @@ const struct prefix * dplane_ctx_rule_get_old_dst_ip(const struct zebra_dplane_ctx *ctx); /* Accessors for policy based routing iptable information */ struct zebra_pbr_iptable; -bool -dplane_ctx_get_pbr_iptable(const struct zebra_dplane_ctx *ctx, - struct zebra_pbr_iptable *table); +void dplane_ctx_get_pbr_iptable(const struct zebra_dplane_ctx *ctx, + struct zebra_pbr_iptable *table); struct zebra_pbr_ipset; void dplane_ctx_get_pbr_ipset(const struct zebra_dplane_ctx *ctx, struct zebra_pbr_ipset *ipset); diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c index b0d274e7c1..8b98cdc688 100644 --- a/zebra/zebra_pbr.c +++ b/zebra/zebra_pbr.c @@ -552,13 +552,12 @@ void zebra_pbr_process_iptable(struct zebra_dplane_ctx *ctx) else mode = 0; - if (dplane_ctx_get_pbr_iptable(ctx, &ipt)) { - ret = hook_call(zebra_pbr_iptable_update, mode, &ipt); - if (ret) - dplane_ctx_set_status(ctx, - ZEBRA_DPLANE_REQUEST_SUCCESS); - } - if (!ret) + dplane_ctx_get_pbr_iptable(ctx, &ipt); + + ret = hook_call(zebra_pbr_iptable_update, mode, &ipt); + if (ret) + dplane_ctx_set_status(ctx, ZEBRA_DPLANE_REQUEST_SUCCESS); + else dplane_ctx_set_status(ctx, ZEBRA_DPLANE_REQUEST_FAILURE); } From f284c1322da96c3ea694b4243b83adaead95ded7 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 28 Oct 2021 10:42:23 -0400 Subject: [PATCH 3/3] zebra: return void for dplane_ctx_get_pbr_ipset_entry The dplane_ctx_get_pbr_ipset_entry function only failed when the caller did not pass in a valid usable pointer. Change the code to assert on a pointer not being passed in and remove the bool return Signed-off-by: Donald Sharp --- zebra/zapi_msg.c | 3 +-- zebra/zebra_dplane.c | 17 ++++++++--------- zebra/zebra_dplane.h | 5 ++--- zebra/zebra_pbr.c | 3 +-- 4 files changed, 12 insertions(+), 16 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 5bacf1f40a..54ab0afd5c 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -940,8 +940,7 @@ void zsend_ipset_entry_notify_owner(const struct zebra_dplane_ctx *ctx, struct zebra_pbr_ipset ipset; uint16_t cmd = ZEBRA_IPSET_ENTRY_NOTIFY_OWNER; - if (!dplane_ctx_get_pbr_ipset_entry(ctx, &ipent)) - return; + dplane_ctx_get_pbr_ipset_entry(ctx, &ipent); dplane_ctx_get_pbr_ipset(ctx, &ipset); if (IS_ZEBRA_DEBUG_PACKET) diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index d02d0178cc..20aa9b8432 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -2171,15 +2171,14 @@ void dplane_ctx_get_pbr_ipset(const struct zebra_dplane_ctx *ctx, memcpy(ipset, &ctx->u.ipset, sizeof(struct zebra_pbr_ipset)); } -bool dplane_ctx_get_pbr_ipset_entry(const struct zebra_dplane_ctx *ctx, +void dplane_ctx_get_pbr_ipset_entry(const struct zebra_dplane_ctx *ctx, struct zebra_pbr_ipset_entry *entry) { DPLANE_CTX_VALID(ctx); - if (!entry) - return false; + assert(entry); + memcpy(entry, &ctx->u.ipset_entry.entry, sizeof(struct zebra_pbr_ipset_entry)); - return true; } /* @@ -5075,12 +5074,12 @@ static void kernel_dplane_log_detail(struct zebra_dplane_ctx *ctx) case DPLANE_OP_IPSET_ENTRY_DELETE: { struct zebra_pbr_ipset_entry ipent; - if (dplane_ctx_get_pbr_ipset_entry(ctx, &ipent)) - zlog_debug("Dplane ipset entry update op %s, unique(%u), ctx %p", - dplane_op2str(dplane_ctx_get_op(ctx)), - ipent.unique, ctx); + dplane_ctx_get_pbr_ipset_entry(ctx, &ipent); + zlog_debug( + "Dplane ipset entry update op %s, unique(%u), ctx %p", + dplane_op2str(dplane_ctx_get_op(ctx)), ipent.unique, + ctx); } break; - case DPLANE_OP_NEIGH_TABLE_UPDATE: zlog_debug("Dplane neigh table op %s, ifp %s, family %s", dplane_op2str(dplane_ctx_get_op(ctx)), diff --git a/zebra/zebra_dplane.h b/zebra/zebra_dplane.h index ea408116ee..977f00bd2a 100644 --- a/zebra/zebra_dplane.h +++ b/zebra/zebra_dplane.h @@ -532,9 +532,8 @@ struct zebra_pbr_ipset; void dplane_ctx_get_pbr_ipset(const struct zebra_dplane_ctx *ctx, struct zebra_pbr_ipset *ipset); struct zebra_pbr_ipset_entry; -bool -dplane_ctx_get_pbr_ipset_entry(const struct zebra_dplane_ctx *ctx, - struct zebra_pbr_ipset_entry *entry); +void dplane_ctx_get_pbr_ipset_entry(const struct zebra_dplane_ctx *ctx, + struct zebra_pbr_ipset_entry *entry); /* Accessors for bridge port information */ uint32_t dplane_ctx_get_br_port_flags(const struct zebra_dplane_ctx *ctx); uint32_t diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c index 8b98cdc688..e66d7aaf5c 100644 --- a/zebra/zebra_pbr.c +++ b/zebra/zebra_pbr.c @@ -591,8 +591,7 @@ void zebra_pbr_process_ipset_entry(struct zebra_dplane_ctx *ctx) else mode = 0; - if (!dplane_ctx_get_pbr_ipset_entry(ctx, &ipset_entry)) - return; + dplane_ctx_get_pbr_ipset_entry(ctx, &ipset_entry); dplane_ctx_get_pbr_ipset(ctx, &ipset); ipset_entry.backpointer = &ipset;