diff --git a/orchagent/cbf/cbfnhgorch.cpp b/orchagent/cbf/cbfnhgorch.cpp index 403945c7a94..f275c5c902d 100644 --- a/orchagent/cbf/cbfnhgorch.cpp +++ b/orchagent/cbf/cbfnhgorch.cpp @@ -308,7 +308,7 @@ bool CbfNhg::sync() nhg_attr.value.u32 = static_cast(m_members.size()); nhg_attrs.push_back(move(nhg_attr)); - if (nhg_attr.value.u32 > gNhgMapOrch->getMaxFcVal()) + if (nhg_attr.value.u32 > gNhgMapOrch->getMaxNumFcs()) { /* If there are more members than FCs then this may be an error, as some members won't be used. */ SWSS_LOG_WARN("More CBF NHG members configured than supported Forwarding Classes"); diff --git a/orchagent/cbf/nhgmaporch.cpp b/orchagent/cbf/nhgmaporch.cpp index d765e3e90ec..fd83fe4b12f 100644 --- a/orchagent/cbf/nhgmaporch.cpp +++ b/orchagent/cbf/nhgmaporch.cpp @@ -294,34 +294,34 @@ void NhgMapOrch::decRefCount(const string &index) } /* - * Get the max FC value supported by the switch. + * Get the maximum number of FC classes supported by the switch. */ -sai_uint8_t NhgMapOrch::getMaxFcVal() +sai_uint8_t NhgMapOrch::getMaxNumFcs() { SWSS_LOG_ENTER(); - static int max_fc_val = -1; + static int max_num_fcs = -1; /* - * Get the maximum value allowed for FC if it wasn't already initialized. + * Get the maximum number of FC classes if it wasn't already initialized. */ - if (max_fc_val == -1) + if (max_num_fcs == -1) { sai_attribute_t attr; attr.id = SAI_SWITCH_ATTR_MAX_NUMBER_OF_FORWARDING_CLASSES; if (sai_switch_api->get_switch_attribute(gSwitchId, 1, &attr) == SAI_STATUS_SUCCESS) { - max_fc_val = attr.value.u8; + max_num_fcs = attr.value.u8; } else { SWSS_LOG_WARN("Switch does not support FCs"); - max_fc_val = 0; + max_num_fcs = 0; } } - return static_cast(max_fc_val); + return static_cast(max_num_fcs); } /* @@ -343,7 +343,7 @@ pair> NhgMapOrch::getMap(const ve } unordered_map fc_map; - sai_uint8_t max_fc_val = getMaxFcVal(); + sai_uint8_t max_num_fcs = getMaxNumFcs(); /* * Create the map while validating that the values are positive @@ -353,13 +353,13 @@ pair> NhgMapOrch::getMap(const ve try { /* - * Check the FC value is valid. + * Check the FC value is valid. FC value must be in range [0, max_num_fcs). */ auto fc = stoi(fvField(*it)); - if ((fc < 0) || (fc > max_fc_val)) + if ((fc < 0) || (fc >= max_num_fcs)) { - SWSS_LOG_ERROR("FC value %d is either negative or greater than max value %d", fc, max_fc_val); + SWSS_LOG_ERROR("FC value %d is either negative or greater than max value %d", fc, max_num_fcs - 1); success = false; break; } diff --git a/orchagent/cbf/nhgmaporch.h b/orchagent/cbf/nhgmaporch.h index c345e7d5668..7d7317a1d6b 100644 --- a/orchagent/cbf/nhgmaporch.h +++ b/orchagent/cbf/nhgmaporch.h @@ -43,9 +43,9 @@ class NhgMapOrch : public Orch void decRefCount(const string &key); /* - * Get the max FC value supported by the switch. + * Get the maximum number of FC classes supported by the switch. */ - static sai_uint8_t getMaxFcVal(); + static sai_uint8_t getMaxNumFcs(); private: /* diff --git a/orchagent/qosorch.cpp b/orchagent/qosorch.cpp index edd5db34432..5a48b699695 100644 --- a/orchagent/qosorch.cpp +++ b/orchagent/qosorch.cpp @@ -840,7 +840,7 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple & { SWSS_LOG_ENTER(); - sai_uint8_t max_fc_val = NhgMapOrch::getMaxFcVal(); + sai_uint8_t max_num_fcs = NhgMapOrch::getMaxNumFcs(); sai_attribute_t list_attr; list_attr.id = SAI_QOS_MAP_ATTR_MAP_TO_VALUE_LIST; @@ -867,10 +867,11 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple & } list_attr.value.qosmap.list[ind].key.dscp = static_cast(value); + // FC value must be in range [0, max_num_fcs) value = stoi(fvValue(*i)); - if ((value < 0) || (value > max_fc_val)) + if ((value < 0) || (value >= max_num_fcs)) { - SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %d", value, max_fc_val); + SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %d", value, max_num_fcs - 1); delete[] list_attr.value.qosmap.list; return false; } @@ -933,7 +934,7 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t { SWSS_LOG_ENTER(); - sai_uint8_t max_fc_val = NhgMapOrch::getMaxFcVal(); + sai_uint8_t max_num_fcs = NhgMapOrch::getMaxNumFcs(); sai_attribute_t list_attr; list_attr.id = SAI_QOS_MAP_ATTR_MAP_TO_VALUE_LIST; @@ -960,10 +961,11 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t } list_attr.value.qosmap.list[ind].key.mpls_exp = static_cast(value); + // FC value must be in range [0, max_num_fcs) value = stoi(fvValue(*i)); - if ((value < 0) || (value > max_fc_val)) + if ((value < 0) || (value >= max_num_fcs)) { - SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %hu", value, max_fc_val); + SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %hu", value, max_num_fcs - 1); delete[] list_attr.value.qosmap.list; return false; } @@ -1692,7 +1694,7 @@ task_process_status QosOrch::handlePortQosMapTable(Consumer& consumer) } sai_uint8_t old_pfc_enable = 0; - if (!gPortsOrch->getPortPfc(port.m_port_id, &old_pfc_enable)) + if (!gPortsOrch->getPortPfc(port.m_port_id, &old_pfc_enable)) { SWSS_LOG_ERROR("Failed to retrieve PFC bits on port %s", port_name.c_str()); } diff --git a/tests/test_nhg.py b/tests/test_nhg.py index df071b4d173..c8d75fb5e7b 100644 --- a/tests/test_nhg.py +++ b/tests/test_nhg.py @@ -1980,7 +1980,7 @@ def data_validation_test(): # Test validation errors nhg_maps = [ ('-1', '0'), # negative FC - ('64', '0'), # greater than max FC value + ('63', '0'), # greater than max FC value ('a', '0'), # non-integer FC ('0', '-1'), # negative NH index ('0', 'a'), # non-integer NH index diff --git a/tests/test_qos_map.py b/tests/test_qos_map.py index 21a25742c95..301bd3c6d62 100644 --- a/tests/test_qos_map.py +++ b/tests/test_qos_map.py @@ -139,7 +139,7 @@ def test_dscp_to_fc(self, dvs): self.init_test(dvs) # Create a DSCP_TO_FC map - dscp_map = [(str(i), str(i)) for i in range(0, 64)] + dscp_map = [(str(i), str(i)) for i in range(0, 63)] self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map)) self.asic_db.wait_for_n_keys(self.ASIC_QOS_MAP_STR, self.asic_qos_map_count + 1) @@ -153,7 +153,7 @@ def test_dscp_to_fc(self, dvs): assert(fvs.get("SAI_QOS_MAP_ATTR_TYPE") == "SAI_QOS_MAP_TYPE_DSCP_TO_FORWARDING_CLASS") # Modify the map - dscp_map = [(str(i), '0') for i in range(0, 64)] + dscp_map = [(str(i), '0') for i in range(0, 63)] self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map)) time.sleep(1) @@ -174,7 +174,7 @@ def test_dscp_to_fc(self, dvs): ('-1', '0'), # negative DSCP ('64', '0'), # DSCP greater than max value ('0', '-1'), # negative FC - ('0', '64'), # FC greater than max value + ('0', '63'), # FC greater than max value ('a', '0'), # non-integer DSCP ('0', 'a'), # non-integet FC ] @@ -228,7 +228,7 @@ def test_exp_to_fc(self, dvs): ('-1', '0'), # negative EXP ('8', '0'), # EXP greater than max value ('0', '-1'), # negative FC - ('0', '64'), # FC greater than max value + ('0', '63'), # FC greater than max value ('a', '0'), # non-integer EXP ('0', 'a'), # non-integet FC ] @@ -258,7 +258,7 @@ def test_per_port_cbf_binding(self, dvs): self.init_test(dvs) # Create a DSCP_TO_FC map - dscp_map = [(str(i), str(i)) for i in range(0, 64)] + dscp_map = [(str(i), str(i)) for i in range(0, 63)] self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map)) self.asic_db.wait_for_n_keys(self.ASIC_QOS_MAP_STR, self.asic_qos_map_count + 1) dscp_map_id = self.get_qos_id()