Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(agw): cpp migration of SPGW files #13213

Closed
wants to merge 10 commits into from

Conversation

pruthvihebbani
Copy link
Contributor

fix(agw): cpp migration of SPGW files

Summary

Migrated all the files under spgw task to cpp/hpp and fixed all the errors and warnings

Test Plan

  • Verified test_oai
  • Verified s1ap sanity suite

… warnings

Signed-off-by: Pruthvi Hebbani <pruthvi.hebbani@radisys.com>
Signed-off-by: Pruthvi Hebbani <pruthvi.hebbani@radisys.com>
Signed-off-by: Pruthvi Hebbani <pruthvi.hebbani@radisys.com>
@pruthvihebbani pruthvihebbani requested review from a team, LKreutzer and ulaskozat July 6, 2022 14:07
@pull-request-size pull-request-size bot added the size/XL Denotes a Pull Request that changes 500-999 lines. label Jul 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added the component: agw Access gateway-related issue label Jul 6, 2022
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

cpplint

lte/gateway/c/core/oai/tasks/sgw/s11_causes.cpp|84| Using C-style cast. Use reinterpret_cast<SGWCauseMapping_t*>(...) instead [readability/casting] [4]
lte/gateway/c/core/oai/tasks/sgw/s11_causes.cpp|88| Using C-style cast. Use const_cast<char*>(...) instead [readability/casting] [4]
lte/gateway/c/core/oai/tasks/sgw/sgw_context_manager.cpp|119| Using C-style cast. Use reinterpret_cast<mme_sgw_tunnel_t*>(...) instead [readability/casting] [4]
lte/gateway/c/core/oai/tasks/sgw/sgw_context_manager.cpp|143| Using C-style cast. Use reinterpret_cast<s_plus_p_gw_eps_bearer_context_information_t*>(...) instead [readability/casting] [4]
lte/gateway/c/core/oai/tasks/sgw/sgw_context_manager.cpp|250| Using C-style cast. Use reinterpret_cast<sgw_eps_bearer_ctxt_t*>(...) instead [readability/casting] [4]
lte/gateway/c/core/oai/tasks/sgw/sgw_handlers.cpp|993| Missing space before ( in if( [whitespace/parens] [5]
lte/gateway/c/core/oai/tasks/sgw/sgw_handlers.cpp|1124| Missing space before ( in if( [whitespace/parens] [5]
lte/gateway/c/core/oai/tasks/sgw/sgw_handlers.cpp|1489| Missing space before ( in if( [whitespace/parens] [5]
lte/gateway/c/core/oai/tasks/sgw/sgw_handlers.cpp|1714| Missing space before ( in if( [whitespace/parens] [5]
lte/gateway/c/core/oai/tasks/sgw/sgw_handlers.cpp|1794| Missing space before ( in if( [whitespace/parens] [5]

sgw_eps_bearer_context_information_t* sgw_ctxt_p,
const itti_gx_nw_init_actv_bearer_request_t* const bearer_req_p,
pdn_type_t pdn_type, uint32_t sgw_ip_address_S1u_S12_S4_up,
struct in6_addr* sgw_ipv6_address_S1u_S12_S4_up, teid_t s1_u_sgw_fteid,
uint32_t sequence_number, log_proto_t module) {
OAILOG_FUNC_IN(module);
sgw_eps_bearer_ctxt_t* eps_bearer_ctxt_p =
calloc(1, sizeof(sgw_eps_bearer_ctxt_t));
(sgw_eps_bearer_ctxt_t*)calloc(1, sizeof(sgw_eps_bearer_ctxt_t));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use reinterpret_cast<sgw_eps_bearer_ctxt_t*>(...) instead [readability/casting] [4]

@@ -670,7 +676,8 @@
}
}
struct sgw_eps_bearer_entry_wrapper_s* sgw_eps_bearer_entry_p =
calloc(1, sizeof(*sgw_eps_bearer_entry_p));
(sgw_eps_bearer_entry_wrapper_s*)calloc(1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use reinterpret_cast<sgw_eps_bearer_entry_wrapper_s*>(...) instead [readability/casting] [4]

@@ -53,19 +59,22 @@ void delete_pending_procedures(
pgw_ni_cbr_proc_t* pgw_create_procedure_create_bearer(
sgw_eps_bearer_context_information_t* ctx_p) {
pgw_ni_cbr_proc_t* s11_proc_create_bearer =
calloc(1, sizeof(pgw_ni_cbr_proc_t));
(pgw_ni_cbr_proc_t*)calloc(1, sizeof(pgw_ni_cbr_proc_t));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use reinterpret_cast<pgw_ni_cbr_proc_t*>(...) instead [readability/casting] [4]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address lint warning

#include "lte/gateway/c/core/oai/lib/3gpp/3gpp_29.274.h"

static const SGWCauseMapping_t causes[] = {
{LOCAL_DETACH, (char*)"Local detach", 0, 0, 0, 0},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use const_cast<char*>(...) instead [readability/casting] [4]


static const SGWCauseMapping_t causes[] = {
{LOCAL_DETACH, (char*)"Local detach", 0, 0, 0, 0},
{COMPLETE_DETACH, (char*)"Complete detach", 0, 0, 0, 0},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use const_cast<char*>(...) instead [readability/casting] [4]

{UE_NOT_RESPONDING, (char*)"UE not responding", 0, 1, 0, 0},
{SERVICE_DENIED, (char*)"Service Denied", 0, 0, 0, 0},
{UNABLE_TO_PAGE_UE, (char*)"Unable to page UE", 0, 1, 0, 0},
{NO_MEMORY_AVAILABLE, (char*)"No memory available", 1, 1, 1, 0},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use const_cast<char*>(...) instead [readability/casting] [4]

{SERVICE_DENIED, (char*)"Service Denied", 0, 0, 0, 0},
{UNABLE_TO_PAGE_UE, (char*)"Unable to page UE", 0, 1, 0, 0},
{NO_MEMORY_AVAILABLE, (char*)"No memory available", 1, 1, 1, 0},
{REQUEST_REJECTED, (char*)"Request rejected", 1, 1, 1, 0},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use const_cast<char*>(...) instead [readability/casting] [4]

{UNABLE_TO_PAGE_UE, (char*)"Unable to page UE", 0, 1, 0, 0},
{NO_MEMORY_AVAILABLE, (char*)"No memory available", 1, 1, 1, 0},
{REQUEST_REJECTED, (char*)"Request rejected", 1, 1, 1, 0},
{INVALID_PEER, (char*)"Invalid peer", 0, 0, 0, 1},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use const_cast<char*>(...) instead [readability/casting] [4]

{REQUEST_REJECTED, (char*)"Request rejected", 1, 1, 1, 0},
{INVALID_PEER, (char*)"Invalid peer", 0, 0, 0, 1},
{TEMP_REJECT_HO_IN_PROGRESS,
(char*)"Temporarily rejected due to HO in progress", 0, 0, 0, 0},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use const_cast<char*>(...) instead [readability/casting] [4]

{INVALID_PEER, (char*)"Invalid peer", 0, 0, 0, 1},
{TEMP_REJECT_HO_IN_PROGRESS,
(char*)"Temporarily rejected due to HO in progress", 0, 0, 0, 0},
{M_PDN_APN_NOT_ALLOWED, (char*)"Multiple PDN for a given APN not allowed",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use const_cast<char*>(...) instead [readability/casting] [4]

Signed-off-by: Pruthvi Hebbani <pruthvi.hebbani@radisys.com>
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

feg-workflow

    2 files  203 suites   39s ⏱️
374 tests 374 ✔️ 0 💤 0
388 runs  388 ✔️ 0 💤 0

Results for commit 75f2796.

♻️ This comment has been updated with latest results.


key.value = cause_value;
res =
(SGWCauseMapping_t*)bsearch((const void*)&key, causes, sizeof(causes),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use reinterpret_cast<SGWCauseMapping_t*>(...) instead [readability/casting] [4]

sizeof(SGWCauseMapping_t), compare_cause_id);

if (res == NULL) {
return (char*)"Unknown cause";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use const_cast<char*>(...) instead [readability/casting] [4]

@@ -110,7 +116,7 @@ mme_sgw_tunnel_t* sgw_cm_create_s11_tunnel(teid_t remote_teid,
{
mme_sgw_tunnel_t* new_tunnel = NULL;

new_tunnel = calloc(1, sizeof(mme_sgw_tunnel_t));
new_tunnel = (mme_sgw_tunnel_t*)calloc(1, sizeof(mme_sgw_tunnel_t));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use reinterpret_cast<mme_sgw_tunnel_t*>(...) instead [readability/casting] [4]

@@ -134,7 +140,8 @@ sgw_cm_create_bearer_context_information_in_collection(teid_t teid) {
s_plus_p_gw_eps_bearer_context_information_t* new_bearer_context_information =
NULL;
new_bearer_context_information =
calloc(1, sizeof(s_plus_p_gw_eps_bearer_context_information_t));
(s_plus_p_gw_eps_bearer_context_information_t*)calloc(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use reinterpret_cast<s_plus_p_gw_eps_bearer_context_information_t*>(...) instead [readability/casting] [4]

@@ -239,7 +246,8 @@ sgw_eps_bearer_ctxt_t* sgw_cm_create_eps_bearer_ctxt_in_collection(

if (!sgw_pdn_connection
->sgw_eps_bearers_array[EBI_TO_INDEX(eps_bearer_idP)]) {
new_eps_bearer_entry = calloc(1, sizeof(sgw_eps_bearer_ctxt_t));
new_eps_bearer_entry =
(sgw_eps_bearer_ctxt_t*)calloc(1, sizeof(sgw_eps_bearer_ctxt_t));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cpplint] reported by reviewdog 🐶
Using C-style cast. Use reinterpret_cast<sgw_eps_bearer_ctxt_t*>(...) instead [readability/casting] [4]

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

dp-workflow

18 tests   18 ✔️  4m 19s ⏱️
  2 suites    0 💤
  2 files      0

Results for commit 75f2796.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

agw-workflow

615 tests   611 ✔️  3m 58s ⏱️
    2 suites      4 💤
    2 files        0

Results for commit 75f2796.

♻️ This comment has been updated with latest results.

@@ -1,89 +0,0 @@
/*
* Licensed to the OpenAirInterface (OAI) Software Alliance under one or more
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move instead of deleting and re-adding

@@ -53,19 +59,22 @@ void delete_pending_procedures(
pgw_ni_cbr_proc_t* pgw_create_procedure_create_bearer(
sgw_eps_bearer_context_information_t* ctx_p) {
pgw_ni_cbr_proc_t* s11_proc_create_bearer =
calloc(1, sizeof(pgw_ni_cbr_proc_t));
(pgw_ni_cbr_proc_t*)calloc(1, sizeof(pgw_ni_cbr_proc_t));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address lint warning

@@ -88,52 +94,41 @@ void populate_sgi_end_point_update(
sgw_eps_bearer_ctxt_t* eps_bearer_ctxt_p,
itti_sgi_update_end_point_response_t* sgi_update_end_point_resp);
bool does_bearer_context_hold_valid_enb_ip(ip_address_t enb_ip_address_S1u);
void populate_sgi_end_point_update(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clean up!

@@ -78,6 +81,9 @@ void sgw_populate_mbr_bearer_contexts_removed(
const itti_sgi_update_end_point_response_t* const resp_pP, imsi64_t imsi64,
sgw_eps_bearer_context_information_t* sgw_context_p,
itti_s11_modify_bearer_response_t* modify_response_p);
void sgw_process_release_access_bearer_request(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this function close to other release access bearer calls

Comment on lines 36 to 37
#include "lte/gateway/c/core/common/dynamic_memory_check.h"
#include "lte/gateway/c/core/common/assertions.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort alphabetically

Suggested change
#include "lte/gateway/c/core/common/dynamic_memory_check.h"
#include "lte/gateway/c/core/common/assertions.h"
#include "lte/gateway/c/core/common/assertions.h"
#include "lte/gateway/c/core/common/dynamic_memory_check.h"

@@ -15,7 +15,7 @@
* contact@openairinterface.org
*/

/*! \file spgw_config.c
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clean up!

Comment on lines 38 to 46
#include "lte/gateway/c/core/oai/common/log.h"
#include "lte/gateway/c/core/oai/lib/itti/intertask_interface.h"
#include "lte/gateway/c/core/oai/lib/itti/intertask_interface_types.h"
#include "lte/gateway/c/core/oai/lib/itti/itti_types.h"
#include "lte/gateway/c/core/oai/common/common_types.h"
#include "lte/gateway/c/core/common/dynamic_memory_check.h"
#include "lte/gateway/c/core/oai/include/sgw_context_manager.h"
extern void print_bearer_ids_helper(const ebi_t*, uint32_t);
#ifdef __cplusplus
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sort alphabetically

imsi64_t imsi64, uint8_t no_of_bearers_to_be_deact,
ebi_t* ebi_to_be_deactivated, bool delete_default_bearer,
teid_t mme_teid_S11, log_proto_t module) {
OAILOG_FUNC_IN(module);
MessageDef* message_p = itti_alloc_new_message(
module, S11_NW_INITIATED_DEACTIVATE_BEARER_REQUEST);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is used by both SPGW task and SGW_S8 task, so need to specify the right sender task based on the module parameter.


message_p =
itti_alloc_new_message(module, S11_NW_INITIATED_ACTIVATE_BEARER_REQUEST);
message_p = itti_alloc_new_message(TASK_SPGW_APP,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the task name related to module ?

Comment on lines 34 to 35
#include "lte/gateway/c/core/oai/tasks/sgw/sgw_defs.hpp"
#include "lte/gateway/c/core/oai/tasks/sgw/sgw_paging.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also sort these alphabetically as part of this change?

@@ -136,7 +143,7 @@ status_code_e pgw_config_process(pgw_config_t* config_pP) {
OAILOG_CRITICAL(
LOG_SPGW_APP,
"ERROR in getting assigned IP block from mobilityd\n");
return -1;
return RETURNerror;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! 👍

sgw_eps_bearer_context_information_t* sgw_ctxt_p,
const itti_gx_nw_init_actv_bearer_request_t* const bearer_req_p,
pdn_type_t pdn_type, uint32_t sgw_ip_address_S1u_S12_S4_up,
struct in6_addr* sgw_ipv6_address_S1u_S12_S4_up, teid_t s1_u_sgw_fteid,
uint32_t sequence_number, log_proto_t module) {
OAILOG_FUNC_IN(module);
sgw_eps_bearer_ctxt_t* eps_bearer_ctxt_p =
calloc(1, sizeof(sgw_eps_bearer_ctxt_t));
(sgw_eps_bearer_ctxt_t*)calloc(1, sizeof(sgw_eps_bearer_ctxt_t));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we could fix all the C-style cast warnings as suggested

"oai/tasks/sgw/sgw_defs.h",
"oai/tasks/sgw/sgw_handlers.h",
"oai/tasks/sgw/sgw_paging.h",
"oai/tasks/sgw/pgw_handlers.hpp",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bazel changes lgtm, thanks for the clean up!

Signed-off-by: Pruthvi Hebbani <pruthvi.hebbani@radisys.com>
Signed-off-by: Pruthvi Hebbani <pruthvi.hebbani@radisys.com>
Signed-off-by: Pruthvi Hebbani <pruthvi.hebbani@radisys.com>
@pruthvihebbani
Copy link
Contributor Author

In this PR I have moved files from c->cpp using mv command due to which the files are treated as new additions and the history is getting lost. Once this PR gets approved, I will close this and create a new PR where I will use git mv to rename the files. Rest of the changes will remain the same.

@ssanadhya
Copy link
Collaborator

In this PR I have moved files from c->cpp using mv command due to which the files are treated as new additions and the history is getting lost. Once this PR gets approved, I will close this and create a new PR where I will use git mv to rename the files. Rest of the changes will remain the same.

Only "s11_causes.c" is showing up as new addition. All other files, e.g. pgw_config.c->pgw_config.cpp, are showing as moved/renamed. So it should be possible to fix this one file in this PR itself. No need to duplicate effort by creating a new PR.

@pruthvihebbani
Copy link
Contributor Author

In this PR I have moved files from c->cpp using mv command due to which the files are treated as new additions and the history is getting lost. Once this PR gets approved, I will close this and create a new PR where I will use git mv to rename the files. Rest of the changes will remain the same.

Only "s11_causes.c" is showing up as new addition. All other files, e.g. pgw_config.c->pgw_config.cpp, are showing as moved/renamed. So it should be possible to fix this one file in this PR itself. No need to duplicate effort by creating a new PR.

They show as renamed but git log does not show the full history of this file. For eg:
image

@ssanadhya
Copy link
Collaborator

In this PR I have moved files from c->cpp using mv command due to which the files are treated as new additions and the history is getting lost. Once this PR gets approved, I will close this and create a new PR where I will use git mv to rename the files. Rest of the changes will remain the same.

Only "s11_causes.c" is showing up as new addition. All other files, e.g. pgw_config.c->pgw_config.cpp, are showing as moved/renamed. So it should be possible to fix this one file in this PR itself. No need to duplicate effort by creating a new PR.

They show as renamed but git log does not show the full history of this file. For eg: image

@pruthvihebbani , Please try git log --follow

@pruthvihebbani
Copy link
Contributor Author

In this PR instead of git mv command mv command was used to rename the files. This was erasing the file commit history. Created a new PR #13264 to address this as modifying this PR was becoming messy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: agw Access gateway-related issue size/XL Denotes a Pull Request that changes 500-999 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants