On 2/7/26 10:48 PM, Alexandra Rukomoinikova wrote:
> Create svc_monitor_type_from_sb() and svc_monitor_proto_from_sb()
> helper functions for cleaner code. Add svc_monitor_type_allows_proto()
> to validate protocol-type compatibility. Simplify ICMP handling
> by removing redundant type checks since validation is now centralized.
>
> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
> ---
Hi Alexandra,
This looks correct and improves readability significantly. Just a few
tiny comments below. But in general I think we should include it with
the rest of the series and accept it for 26.03.
> controller/pinctrl.c | 91 +++++++++++++++++++++++++++++---------------
> 1 file changed, 60 insertions(+), 31 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 7b80f4725..476410dcc 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -7021,6 +7021,55 @@ pinctrl_find_svc_monitor(uint32_t dp_key, uint32_t
> port_key,
> return NULL;
> }
>
> +static enum svc_monitor_type
> +svc_monitor_type_from_sb(const struct sbrec_service_monitor *sb)
> +{
> + if (sb->type &&
> + !strcmp(sb->type, "network-function")) {
> + return SVC_MON_TYPE_NF;
> + }
> +
> + if (sb->type &&
> + !strcmp(sb->type, "logical-switch-port")) {
> + return SVC_MON_TYPE_LSP;
> + }
> +
> + return SVC_MON_TYPE_LB;
> +}
> +
> +static enum svc_monitor_protocol
> +svc_monitor_proto_from_sb(const struct sbrec_service_monitor *sb)
> +{
> + if (!strcmp(sb->protocol, "udp")) {
> + return SVC_MON_PROTO_UDP;
> + }
> +
> + if (!strcmp(sb->protocol, "icmp")) {
> + return SVC_MON_PROTO_ICMP;
> + }
> +
> + return SVC_MON_PROTO_TCP;
> +}
> +
> +static bool
> +svc_monitor_type_allows_proto(enum svc_monitor_type type,
> + enum svc_monitor_protocol proto)
> +{
> + switch (type) {
> + case SVC_MON_TYPE_NF:
> + return proto == SVC_MON_PROTO_ICMP;
> +
> + case SVC_MON_TYPE_LB:
> + return proto == SVC_MON_PROTO_TCP ||
> + proto == SVC_MON_PROTO_UDP;
> +
> + case SVC_MON_TYPE_LSP:
> + return true;
Maybe:
default:
OVS_NOT_REACHED()
?
> + }
> +
> + return false;
> +}
> +
> static void
> sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
> const struct sbrec_service_monitor_table *svc_mon_table,
> @@ -7037,26 +7086,6 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
>
> const struct sbrec_service_monitor *sb_svc_mon;
> SBREC_SERVICE_MONITOR_TABLE_FOR_EACH (sb_svc_mon, svc_mon_table) {
> - enum svc_monitor_type mon_type;
> - if (sb_svc_mon->type && !strcmp(sb_svc_mon->type,
> - "network-function")) {
> - mon_type = SVC_MON_TYPE_NF;
> - } else if (sb_svc_mon->type && !strcmp(sb_svc_mon->type,
> - "logical-switch-port")) {
> - mon_type = SVC_MON_TYPE_LSP;
> - } else {
> - mon_type = SVC_MON_TYPE_LB;
> - }
> -
> - enum svc_monitor_protocol protocol;
> - if (!strcmp(sb_svc_mon->protocol, "udp")) {
> - protocol = SVC_MON_PROTO_UDP;
> - } else if (!strcmp(sb_svc_mon->protocol, "icmp")) {
> - protocol = SVC_MON_PROTO_ICMP;
> - } else {
> - protocol = SVC_MON_PROTO_TCP;
> - }
> -
> const struct sbrec_port_binding *pb
> = lport_lookup_by_name(sbrec_port_binding_by_name,
> sb_svc_mon->logical_port);
> @@ -7090,10 +7119,16 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct eth_addr ea;
> bool mac_found = false;
>
> + enum svc_monitor_type mon_type =
> + svc_monitor_type_from_sb(sb_svc_mon);
> + enum svc_monitor_protocol protocol =
> + svc_monitor_proto_from_sb(sb_svc_mon);
> +
> + if (!svc_monitor_type_allows_proto(mon_type, protocol)) {
> + continue;
> + }
> +
> if (mon_type == SVC_MON_TYPE_NF) {
> - if (protocol != SVC_MON_PROTO_ICMP) {
> - continue;
> - }
> input_pb = lport_lookup_by_name(sbrec_port_binding_by_name,
> sb_svc_mon->logical_input_port);
> if (!input_pb) {
> @@ -8432,10 +8467,7 @@ pinctrl_handle_svc_check(struct rconn *swconn, const
> struct flow *ip_flow,
> "request" : "reply");
> return;
> }
> - if (svc_mon->type == SVC_MON_TYPE_NF ||
> - svc_mon->type == SVC_MON_TYPE_LSP) {
> - pinctrl_handle_icmp_svc_check(pkt_in, svc_mon);
> - }
> + pinctrl_handle_icmp_svc_check(pkt_in, svc_mon);
As you mentioned in the discussion on patch 3/8, this chunk could go there.
> return;
> }
> } else if (in_eth->eth_type == htons(ETH_TYPE_IPV6)) {
> @@ -8461,10 +8493,7 @@ pinctrl_handle_svc_check(struct rconn *swconn, const
> struct flow *ip_flow,
> "request" : "reply");
> return;
> }
> - if (svc_mon->type == SVC_MON_TYPE_NF ||
> - svc_mon->type == SVC_MON_TYPE_LSP) {
> - pinctrl_handle_icmp_svc_check(pkt_in, svc_mon);
> - }
> + pinctrl_handle_icmp_svc_check(pkt_in, svc_mon);
As you mentioned in the discussion on patch 3/8, this chunk could go there.
> return;
> }
> }
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
