From 17b4de2fe1216b16523ab43e570c3e62328b893b Mon Sep 17 00:00:00 2001 From: Marie <37634144+themarwhal@users.noreply.github.com> Date: Fri, 14 May 2021 12:45:44 -0500 Subject: [PATCH] refactor: Cleanup ChargingGrant output parameter types (#6935) Signed-off-by: Marie Bremner --- .../c/session_manager/ChargingGrant.cpp | 73 ++++++++++--------- lte/gateway/c/session_manager/ChargingGrant.h | 30 +++----- .../c/session_manager/SessionState.cpp | 64 ++++++++-------- .../test/test_charging_grant.cpp | 18 ++--- 4 files changed, 89 insertions(+), 96 deletions(-) diff --git a/lte/gateway/c/session_manager/ChargingGrant.cpp b/lte/gateway/c/session_manager/ChargingGrant.cpp index 1907c6129d19..56ab8ee9c83d 100644 --- a/lte/gateway/c/session_manager/ChargingGrant.cpp +++ b/lte/gateway/c/session_manager/ChargingGrant.cpp @@ -100,9 +100,10 @@ CreditValidity ChargingGrant::is_valid_credit_response( } void ChargingGrant::receive_charging_grant( - const CreditUpdateResponse& update, SessionCreditUpdateCriteria* uc) { + const CreditUpdateResponse& update, + SessionCreditUpdateCriteria* credit_uc) { auto p_credit = update.credit(); - credit.receive_credit(p_credit.granted_units(), uc); + credit.receive_credit(p_credit.granted_units(), credit_uc); // Final Action is_final_grant = p_credit.is_final(); @@ -133,36 +134,36 @@ void ChargingGrant::receive_charging_grant( } log_received_grant(update); - // Update the UpdateCriteria if not NULL - if (uc != NULL) { - uc->is_final = is_final_grant; - uc->final_action_info = final_action_info; - uc->expiry_time = expiry_time; - uc->suspended = suspended; + // Update the UpdateCriteria if not nullptr + if (credit_uc != nullptr) { + credit_uc->is_final = is_final_grant; + credit_uc->final_action_info = final_action_info; + credit_uc->expiry_time = expiry_time; + credit_uc->suspended = suspended; } } SessionCreditUpdateCriteria ChargingGrant::get_update_criteria() { - SessionCreditUpdateCriteria uc = credit.get_update_criteria(); - uc.is_final = is_final_grant; - uc.final_action_info = final_action_info; - uc.expiry_time = expiry_time; - uc.reauth_state = reauth_state; - uc.service_state = service_state; - uc.suspended = suspended; - return uc; + SessionCreditUpdateCriteria credit_uc = credit.get_update_criteria(); + credit_uc.is_final = is_final_grant; + credit_uc.final_action_info = final_action_info; + credit_uc.expiry_time = expiry_time; + credit_uc.reauth_state = reauth_state; + credit_uc.service_state = service_state; + credit_uc.suspended = suspended; + return credit_uc; } CreditUsage ChargingGrant::get_credit_usage( - CreditUsage::UpdateType update_type, SessionCreditUpdateCriteria* uc, + CreditUsage::UpdateType update_type, SessionCreditUpdateCriteria* credit_uc, bool is_terminate) { CreditUsage p_usage; Usage credit_usage; if (is_final_grant || is_terminate) { - credit_usage = credit.get_all_unreported_usage_for_reporting(uc); + credit_usage = credit.get_all_unreported_usage_for_reporting(credit_uc); } else { - credit_usage = credit.get_usage_for_reporting(uc); + credit_usage = credit.get_usage_for_reporting(credit_uc); } p_usage.set_bytes_tx(credit_usage.bytes_tx); p_usage.set_bytes_rx(credit_usage.bytes_rx); @@ -230,19 +231,20 @@ bool ChargingGrant::should_deactivate_service() const { return false; } -ServiceActionType ChargingGrant::get_action(SessionCreditUpdateCriteria& uc) { +ServiceActionType ChargingGrant::get_action( + SessionCreditUpdateCriteria* credit_uc) { switch (service_state) { case SERVICE_NEEDS_DEACTIVATION: - set_service_state(SERVICE_DISABLED, uc); + set_service_state(SERVICE_DISABLED, credit_uc); if (!is_final_grant) { return TERMINATE_SERVICE; } return final_action_to_action(final_action_info.final_action); case SERVICE_NEEDS_ACTIVATION: - set_service_state(SERVICE_ENABLED, uc); + set_service_state(SERVICE_ENABLED, credit_uc); return ACTIVATE_SERVICE; case SERVICE_NEEDS_SUSPENSION: - set_service_state(SERVICE_DISABLED, uc); + set_service_state(SERVICE_DISABLED, credit_uc); return final_action_to_action_on_suspension( final_action_info.final_action); default: @@ -277,34 +279,39 @@ ServiceActionType ChargingGrant::final_action_to_action_on_suspension( } void ChargingGrant::set_reauth_state( - const ReAuthState new_state, SessionCreditUpdateCriteria& uc) { + const ReAuthState new_state, SessionCreditUpdateCriteria* credit_uc) { if (reauth_state != new_state) { MLOG(MDEBUG) << "ReAuth state change from " << reauth_state_to_str(reauth_state) << " to " << reauth_state_to_str(new_state); } - reauth_state = new_state; - uc.reauth_state = new_state; + reauth_state = new_state; + if (credit_uc != nullptr) { + credit_uc->reauth_state = new_state; + } } void ChargingGrant::set_service_state( - const ServiceState new_service_state, SessionCreditUpdateCriteria& uc) { + const ServiceState new_service_state, + SessionCreditUpdateCriteria* credit_uc) { if (service_state != new_service_state) { MLOG(MDEBUG) << "Service state change from " << service_state_to_str(service_state) << " to " << service_state_to_str(new_service_state); } - service_state = new_service_state; - uc.service_state = new_service_state; + service_state = new_service_state; + if (credit_uc != nullptr) { + credit_uc->service_state = new_service_state; + } } void ChargingGrant::set_suspended( - bool new_suspended, SessionCreditUpdateCriteria* uc) { + bool new_suspended, SessionCreditUpdateCriteria* credit_uc) { if (suspended != new_suspended) { MLOG(MDEBUG) << "Credit suspension set to: " << new_suspended; } - suspended = new_suspended; - uc->suspended = new_suspended; + suspended = new_suspended; + credit_uc->suspended = new_suspended; } bool ChargingGrant::get_suspended() { @@ -315,7 +322,7 @@ void ChargingGrant::reset_reporting_grant( SessionCreditUpdateCriteria* credit_uc) { credit.reset_reporting_credit(credit_uc); if (reauth_state == REAUTH_PROCESSING) { - set_reauth_state(REAUTH_REQUIRED, *credit_uc); + set_reauth_state(REAUTH_REQUIRED, credit_uc); } } diff --git a/lte/gateway/c/session_manager/ChargingGrant.h b/lte/gateway/c/session_manager/ChargingGrant.h index 499d7fe80698..1b166270b7dc 100644 --- a/lte/gateway/c/session_manager/ChargingGrant.h +++ b/lte/gateway/c/session_manager/ChargingGrant.h @@ -57,14 +57,14 @@ struct ChargingGrant { reauth_state(REAUTH_NOT_NEEDED), suspended(false) {} - ChargingGrant(const StoredChargingGrant& marshaled); + explicit ChargingGrant(const StoredChargingGrant& marshaled); // ChargingGrant -> StoredChargingGrant StoredChargingGrant marshal(); void receive_charging_grant( const CreditUpdateResponse& update, - SessionCreditUpdateCriteria* uc = NULL); + SessionCreditUpdateCriteria* credit_uc = NULL); // Returns true if the credit returned from the Policy component is valid and // good to be installed. @@ -82,7 +82,7 @@ struct ChargingGrant { // get_action returns the action to take on the credit based on the last // update. If no action needs to take place, CONTINUE_SERVICE is returned. - ServiceActionType get_action(SessionCreditUpdateCriteria& update_criteria); + ServiceActionType get_action(SessionCreditUpdateCriteria* update_criteria); // Get unreported usage from credit and return as part of CreditUsage // The update_type is also included in CreditUsage @@ -90,12 +90,8 @@ struct ChargingGrant { // usage, otherwise we only include unreported usage up to the allocated // amount. CreditUsage get_credit_usage( - CreditUsage::UpdateType update_type, SessionCreditUpdateCriteria* uc, - bool is_terminate); - - // get_requested_units returns total, tx and rx needed to cover one worth of - // grant - RequestedUnits get_requested_units(); + CreditUsage::UpdateType update_type, + SessionCreditUpdateCriteria* credit_uc, bool is_terminate); // Return true if the service needs to be deactivated bool should_deactivate_service() const; @@ -107,22 +103,18 @@ struct ChargingGrant { ServiceActionType final_action_to_action_on_suspension( const ChargingCredit_FinalAction action) const; - // Set is_final_grant and final_action_info values - void set_final_action_info( - const magma::lte::ChargingCredit& credit, - SessionCreditUpdateCriteria* uc = NULL); - bool get_suspended(); - void set_suspended(bool suspended, SessionCreditUpdateCriteria* uc); + void set_suspended(bool suspended, SessionCreditUpdateCriteria* credit_uc); // Set the object and update criteria's reauth state to new_state. void set_reauth_state( - const ReAuthState new_state, SessionCreditUpdateCriteria& uc); + const ReAuthState new_state, SessionCreditUpdateCriteria* credit_uc); // Set the object and update criteria's service state to new_state. void set_service_state( - const ServiceState new_service_state, SessionCreditUpdateCriteria& uc); + const ServiceState new_service_state, + SessionCreditUpdateCriteria* credit_uc); // Set the flag reporting. Used to signal this credit is waiting to receive // a response from the core @@ -131,10 +123,6 @@ struct ChargingGrant { // Rollback reporting changes for failed updates void reset_reporting_grant(SessionCreditUpdateCriteria* credit_uc); - // Convert rel_time_sec, which is a delta value in seconds, into a timestamp - // and assign it to expiry_time - void set_expiry_time_as_timestamp(uint32_t rel_time_sec); - // Log information about the grant received void log_received_grant(const CreditUpdateResponse& update); }; diff --git a/lte/gateway/c/session_manager/SessionState.cpp b/lte/gateway/c/session_manager/SessionState.cpp index 5bd63a2ce291..608fe3d755ab 100644 --- a/lte/gateway/c/session_manager/SessionState.cpp +++ b/lte/gateway/c/session_manager/SessionState.cpp @@ -471,10 +471,11 @@ void SessionState::add_rule_usage( << " Service Identifier=" << charging_key.service_identifier; auto it = credit_map_.find(charging_key); if (it != credit_map_.end()) { - auto credit_uc = get_credit_uc(charging_key, update_criteria); + SessionCreditUpdateCriteria* credit_uc = + get_credit_uc(charging_key, update_criteria); it->second->credit.add_used_credit(used_tx, used_rx, credit_uc); if (it->second->should_deactivate_service()) { - it->second->set_service_state(SERVICE_NEEDS_DEACTIVATION, *credit_uc); + it->second->set_service_state(SERVICE_NEEDS_DEACTIVATION, credit_uc); } } else { MLOG(MDEBUG) << "Rating Group " << charging_key.rating_group @@ -740,7 +741,8 @@ SessionTerminateRequest SessionState::make_termination_request( } // gy credits for (auto& credit_pair : credit_map_) { - auto credit_uc = get_credit_uc(credit_pair.first, uc); + SessionCreditUpdateCriteria* credit_uc = + get_credit_uc(credit_pair.first, uc); auto credit_usage = credit_pair.second->get_credit_usage( CreditUsage::TERMINATED, credit_uc, true); credit_pair.first.set_credit_usage(&credit_usage); @@ -1393,8 +1395,8 @@ void SessionState::suspend_service_if_needed_for_credit( } } if (credit_map_.size() > 0 && suspended_count == credit_map_.size()) { - auto credit_uc = get_credit_uc(ckey, update_criteria); - it->second->set_service_state(SERVICE_NEEDS_SUSPENSION, *credit_uc); + it->second->set_service_state( + SERVICE_NEEDS_SUSPENSION, get_credit_uc(ckey, update_criteria)); } } @@ -1510,14 +1512,14 @@ bool SessionState::receive_charging_credit( // new credit return init_charging_credit(update, session_uc); } - auto& grant = it->second; - auto credit_uc = get_credit_uc(key, session_uc); + auto& grant = it->second; + SessionCreditUpdateCriteria* credit_uc = get_credit_uc(key, session_uc); auto credit_validity = ChargingGrant::is_valid_credit_response(update); if (credit_validity == INVALID_CREDIT) { // update unsuccessful, reset credit and return grant->credit.mark_failure(update.result_code(), credit_uc); if (grant->should_deactivate_service()) { - grant->set_service_state(SERVICE_NEEDS_DEACTIVATION, *credit_uc); + grant->set_service_state(SERVICE_NEEDS_DEACTIVATION, credit_uc); } return false; } @@ -1529,7 +1531,7 @@ bool SessionState::receive_charging_credit( grant->receive_charging_grant(update, credit_uc); if (grant->reauth_state == REAUTH_PROCESSING) { - grant->set_reauth_state(REAUTH_NOT_NEEDED, *credit_uc); + grant->set_reauth_state(REAUTH_NOT_NEEDED, credit_uc); } if (!grant->credit.is_quota_exhausted(1) && grant->service_state != SERVICE_ENABLED) { @@ -1537,7 +1539,7 @@ bool SessionState::receive_charging_credit( MLOG(MINFO) << "New quota now available. Service is in state: " << service_state_to_str(grant->service_state) << " Activating service RG: " << key << " for " << session_id_; - grant->set_service_state(SERVICE_NEEDS_ACTIVATION, *credit_uc); + grant->set_service_state(SERVICE_NEEDS_ACTIVATION, credit_uc); } return true; } @@ -1567,9 +1569,9 @@ void SessionState::set_suspend_credit( SessionStateUpdateCriteria& update_criteria) { auto it = credit_map_.find(charging_key); if (it != credit_map_.end()) { - auto credit_uc = get_credit_uc(charging_key, update_criteria); - auto& grant = it->second; - grant->set_suspended(new_suspended, credit_uc); + auto& grant = it->second; + grant->set_suspended( + new_suspended, get_credit_uc(charging_key, update_criteria)); } } @@ -1625,16 +1627,14 @@ bool SessionState::set_credit_reporting( } it->second->credit.set_reporting(reporting); - if (session_uc != NULL) { - auto credit_uc = get_credit_uc(key, *session_uc); - credit_uc->reporting = reporting; + if (session_uc != nullptr) { + get_credit_uc(key, *session_uc)->reporting = reporting; } return true; } ReAuthResult SessionState::reauth_key( - const CreditKey& charging_key, - SessionStateUpdateCriteria& update_criteria) { + const CreditKey& charging_key, SessionStateUpdateCriteria& session_uc) { auto it = credit_map_.find(charging_key); if (it != credit_map_.end()) { // if credit is already reporting, don't initiate update @@ -1642,9 +1642,8 @@ ReAuthResult SessionState::reauth_key( if (grant->credit.is_reporting()) { return ReAuthResult::UPDATE_NOT_NEEDED; } - auto uc = grant->get_update_criteria(); - grant->set_reauth_state(REAUTH_REQUIRED, uc); - update_criteria.charging_credit_map[charging_key] = uc; + grant->set_reauth_state( + REAUTH_REQUIRED, get_credit_uc(charging_key, session_uc)); return ReAuthResult::UPDATE_INITIATED; } // charging_key cannot be found, initialize credit and engage reauth @@ -1652,8 +1651,8 @@ ReAuthResult SessionState::reauth_key( grant->credit = SessionCredit(SERVICE_DISABLED); grant->reauth_state = REAUTH_REQUIRED; grant->service_state = SERVICE_DISABLED; - update_criteria.charging_credit_to_install[charging_key] = grant->marshal(); - credit_map_[charging_key] = std::move(grant); + session_uc.charging_credit_to_install[charging_key] = grant->marshal(); + credit_map_[charging_key] = std::move(grant); return ReAuthResult::UPDATE_INITIATED; } @@ -1665,9 +1664,8 @@ ReAuthResult SessionState::reauth_all( auto& grant = credit_pair.second; // Only update credits that aren't reporting if (!grant->credit.is_reporting()) { - update_criteria.charging_credit_map[key] = grant->get_update_criteria(); grant->set_reauth_state( - REAUTH_REQUIRED, update_criteria.charging_credit_map[key]); + REAUTH_REQUIRED, get_credit_uc(key, update_criteria)); res = ReAuthResult::UPDATE_INITIATED; } } @@ -1740,11 +1738,11 @@ void SessionState::get_charging_updates( std::vector>* actions_out, SessionStateUpdateCriteria& uc) { for (auto& credit_pair : credit_map_) { - auto& key = credit_pair.first; - auto& grant = credit_pair.second; - auto credit_uc = get_credit_uc(key, uc); + auto& key = credit_pair.first; + auto& grant = credit_pair.second; + SessionCreditUpdateCriteria* credit_uc = get_credit_uc(key, uc); - auto action_type = grant->get_action(*credit_uc); + auto action_type = grant->get_action(credit_uc); auto action = std::make_unique(action_type); switch (action_type) { case CONTINUE_SERVICE: { @@ -1761,7 +1759,7 @@ void SessionState::get_charging_updates( MLOG(MDEBUG) << "Redirection already activated for " << session_id_; continue; } - grant->set_service_state(SERVICE_REDIRECTED, *credit_uc); + grant->set_service_state(SERVICE_REDIRECTED, credit_uc); PolicyRule redirect_rule = make_redirect_rule(grant); if (!is_gy_dynamic_rule_installed(redirect_rule.id())) { @@ -1777,7 +1775,7 @@ void SessionState::get_charging_updates( MLOG(MDEBUG) << "Restriction already activated for " << session_id_; continue; } - grant->set_service_state(SERVICE_RESTRICTED, *credit_uc); + grant->set_service_state(SERVICE_RESTRICTED, credit_uc); fill_service_action_for_restrict(action, key, grant, uc); actions_out->push_back(std::move(action)); @@ -1827,9 +1825,9 @@ optional SessionState::get_update_for_continue_service( << credit_update_type_to_str(update_type) << " with request number " << request_number_; - auto credit_uc = get_credit_uc(key, session_uc); + SessionCreditUpdateCriteria* credit_uc = get_credit_uc(key, session_uc); if (update_type == CreditUsage::REAUTH_REQUIRED) { - grant->set_reauth_state(REAUTH_PROCESSING, *credit_uc); + grant->set_reauth_state(REAUTH_PROCESSING, credit_uc); } CreditUsage usage = grant->get_credit_usage(update_type, credit_uc, false); key.set_credit_usage(&usage); diff --git a/lte/gateway/c/session_manager/test/test_charging_grant.cpp b/lte/gateway/c/session_manager/test/test_charging_grant.cpp index c5449073d87e..bccc94be4493 100644 --- a/lte/gateway/c/session_manager/test/test_charging_grant.cpp +++ b/lte/gateway/c/session_manager/test/test_charging_grant.cpp @@ -189,7 +189,7 @@ TEST_F(ChargingGrantTest, test_get_action) { grant.is_final_grant = false; grant.credit.receive_credit(gsu, &uc); grant.credit.add_used_credit(1024, 0, &uc); - auto cont_action = grant.get_action(uc); + auto cont_action = grant.get_action(&uc); EXPECT_EQ(cont_action, CONTINUE_SERVICE); // Check both the grant's service_state and update criteria EXPECT_EQ(grant.service_state, CONTINUE_SERVICE); @@ -203,12 +203,12 @@ TEST_F(ChargingGrantTest, test_get_action) { grant.credit.add_used_credit(2048, 0, &uc); grant.credit.add_used_credit(30, 20, &uc); grant.service_state = SERVICE_NEEDS_DEACTIVATION; - auto term_action = grant.get_action(uc); + auto term_action = grant.get_action(&uc); // Check that the update criteria also includes the changes EXPECT_EQ(term_action, TERMINATE_SERVICE); // Termination action only returned once - auto repeated_action = grant.get_action(uc); + auto repeated_action = grant.get_action(&uc); EXPECT_EQ(repeated_action, CONTINUE_SERVICE); } @@ -227,12 +227,12 @@ TEST_F(ChargingGrantTest, test_get_action_redirect) { grant.credit.add_used_credit(2048, 0, &uc); grant.credit.add_used_credit(30, 20, &uc); grant.service_state = SERVICE_NEEDS_DEACTIVATION; - auto term_action = grant.get_action(uc); + auto term_action = grant.get_action(&uc); // Check that the update criteria also includes the changes EXPECT_EQ(term_action, REDIRECT); // Termination action only returned once - auto repeated_action = grant.get_action(uc); + auto repeated_action = grant.get_action(&uc); EXPECT_EQ(repeated_action, CONTINUE_SERVICE); } @@ -251,12 +251,12 @@ TEST_F(ChargingGrantTest, test_get_action_restrict) { grant.credit.add_used_credit(2048, 0, &uc); grant.credit.add_used_credit(30, 20, &uc); grant.service_state = SERVICE_NEEDS_DEACTIVATION; - auto term_action = grant.get_action(uc); + auto term_action = grant.get_action(&uc); // Check that the update criteria also includes the changes EXPECT_EQ(term_action, RESTRICT_ACCESS); // Termination action only returned once - auto repeated_action = grant.get_action(uc); + auto repeated_action = grant.get_action(&uc); EXPECT_EQ(repeated_action, CONTINUE_SERVICE); } @@ -280,7 +280,7 @@ TEST_F(ChargingGrantTest, test_tolerance_quota_exhausted) { EXPECT_EQ(uc.bucket_deltas[USED_TX], 2000); EXPECT_TRUE(credit.is_quota_exhausted(0.8)); // continue the service even we are over the quota (but not final unit) - EXPECT_EQ(grant.get_action(uc), CONTINUE_SERVICE); + EXPECT_EQ(grant.get_action(&uc), CONTINUE_SERVICE); // Check how much we will report (we will report everything, even if // we go have gone over the allowed quota) @@ -331,7 +331,7 @@ TEST_F(ChargingGrantTest, test_tolerance_quota_exhausted) { EXPECT_EQ(credit.get_credit(USED_TX), 4000); EXPECT_TRUE(credit.is_quota_exhausted(1)); // 100% exceeded EXPECT_TRUE(grant.should_deactivate_service()); - grant.set_service_state(SERVICE_NEEDS_DEACTIVATION, uc); + grant.set_service_state(SERVICE_NEEDS_DEACTIVATION, &uc); // Since this is the final grant, we should not report anything EXPECT_FALSE(grant.get_update_type(&update_type));