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

[AGW] [MME] Adding IP_ALLOCATION_RESPONSE to handle ip allocation process on SPGW… #5416

Merged
merged 3 commits into from
Mar 12, 2021

Conversation

ssanadhya
Copy link
Collaborator

… main thread

Signed-off-by: Alex Rodriguez ardzoht@gmail.com

Summary

Test Plan

Additional Information

  • This change is backwards-breaking

@magmabot magmabot added the component: agw Access gateway-related issue label Mar 10, 2021
@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #5416 (250ce2e) into master (82fa960) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5416   +/-   ##
=======================================
  Coverage   63.58%   63.58%           
=======================================
  Files         301      301           
  Lines       20376    20376           
=======================================
  Hits        12957    12957           
  Misses       5514     5514           
  Partials     1905     1905           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82fa960...1398b43. Read the comment docs.

@ardzoht ardzoht force-pushed the add_ip_allocation_rsp_itti_msg branch from 250ce2e to 1398b43 Compare March 10, 2021 21:34
@rdefosse
Copy link
Contributor

MAGMA-OAI-TESTS

MobilityServiceClient::getInstance().AllocateIPv4AddressAsync(
subscriber_id, apn,
[=, &s5_response](
const Status& status, AllocateIPAddressResponse ip_msg) {
[addr, subscriber_id, apn, pdn_type, context_teid, eps_bearer_id](
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do not need to capture the addr from the caller scope here. It was originally used to capture the addr allocated by the RPC, but this will be taken care of through the ITTI message now.

});
return 0;
}

static itti_sgi_create_end_point_response_t handle_allocate_ipv4_address_status(
static int handle_allocate_ipv4_address_status(
Copy link
Contributor

Choose a reason for hiding this comment

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

Since caller is ignoring the return status, do you still need it?

s_plus_p_gw_eps_bearer_context_information_t* bearer_ctxt_info_p =
sgw_cm_get_spgw_context(ip_allocation_rsp->context_teid);

if (bearer_ctxt_info_p) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add error or warning msg if context is not found

Comment on lines 1944 to 1954
if (ip_allocation_rsp->status == SGI_STATUS_ERROR_SYSTEM_FAILURE) {
/*
* This implies that UE session was not release properly.
* Release the IP address so that subsequent attempt is successfull
*/
// TODO - Release the GTP-tunnel corresponding to this IP address
char* apn =
(char*) bearer_ctxt_info_p->sgw_eps_bearer_context_information
.pdn_connection.apn_in_use;
release_ipv4_address(imsi, apn, &ip_allocation_rsp->paa.ipv4_address);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for not leaving this logic inside the function "handle_allocate_ipv4_address_status" to simplify this handler

… main thread

Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
@ardzoht ardzoht force-pushed the add_ip_allocation_rsp_itti_msg branch from 1398b43 to 8544622 Compare March 11, 2021 20:22
MobilityServiceClient::getInstance().AllocateIPv4AddressAsync(
subscriber_id, apn,
[=, &s5_response](
const Status& status, AllocateIPAddressResponse ip_msg) {
[subscriber_id, apn, pdn_type, context_teid, eps_bearer_id](
Copy link
Contributor

@ulaskozat ulaskozat Mar 11, 2021

Choose a reason for hiding this comment

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

is passing subscriber_id and apn that are pointers to the lambda function a safe call? Can what they are pointing to be deleted by the spgw thread?

Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed on the call, let's define std::string objects and pass those instead.

Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
@ardzoht ardzoht force-pushed the add_ip_allocation_rsp_itti_msg branch from bf4e293 to aec144a Compare March 11, 2021 21:57
@ssanadhya
Copy link
Collaborator Author

MAGMA-OAI-TESTS

@ssanadhya
Copy link
Collaborator Author

ssanadhya commented Mar 11, 2021

LGTM, but I cannot approve it as I created the PR on behalf of @ardzoht, waiting for approval from other code-owners.

Copy link
Contributor

@ulaskozat ulaskozat left a comment

Choose a reason for hiding this comment

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

LGTM

@ulaskozat ulaskozat added the apply-v1.4 Needs to be applied to v1.4 release branch as well label Mar 12, 2021
@ulaskozat ulaskozat merged commit 9c19fcd into magma:master Mar 12, 2021
@themarwhal themarwhal added the backported-v1.4 Has been backported to v1.4 release branch label Mar 12, 2021
themarwhal pushed a commit that referenced this pull request Mar 12, 2021
…cess on SPGW… (#5416)

* Adding IP_ALLOCATION_RESPONSE to handle ip allocation process on SPGW main thread

Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
@ssanadhya ssanadhya linked an issue Mar 16, 2021 that may be closed by this pull request
chandra-77 pushed a commit to chandra-77/magma that referenced this pull request Mar 30, 2021
…cess on SPGW… (magma#5416)

* Adding IP_ALLOCATION_RESPONSE to handle ip allocation process on SPGW main thread

Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apply-v1.4 Needs to be applied to v1.4 release branch as well backported-v1.4 Has been backported to v1.4 release branch component: agw Access gateway-related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MME] SEGV in cpp_redis::network::redis_connection::send
7 participants