[M] Change in osmo-mgw[master]: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
pespin has submitted this change. (
https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email )
Change subject: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
..
mgcp-client: Transmit remote IP addr in CRCX if known and port=0
A client may know the IP address during CRCX but not yet the port, or it
may simply want to hint an initial IP address so that the MGW can better
guess when allocating a local IP address.
Related: SYS#6657
Change-Id: I30165dbac5e484011d0acf46af36f105954a501d
---
M src/libosmo-mgcp-client/mgcp_client.c
M src/libosmo-mgcp-client/mgcp_client_fsm.c
2 files changed, 56 insertions(+), 31 deletions(-)
Approvals:
osmith: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
neels: Looks good to me, but someone else must approve
Jenkins Builder: Verified
diff --git a/src/libosmo-mgcp-client/mgcp_client.c
b/src/libosmo-mgcp-client/mgcp_client.c
index ceb9592..8693b1b 100644
--- a/src/libosmo-mgcp-client/mgcp_client.c
+++ b/src/libosmo-mgcp-client/mgcp_client.c
@@ -1302,9 +1302,6 @@
local_ip_family = osmo_ip_str_type(local_ip);
if (local_ip_family == AF_UNSPEC)
return -EINVAL;
- audio_ip_family = osmo_ip_str_type(mgcp_msg->audio_ip);
- if (audio_ip_family == AF_UNSPEC)
- return -EINVAL;
/* Add owner/creator (SDP) */
MSGB_PRINTF_OR_RET("o=- %x 23 IN IP%c %s\r\n", mgcp_msg->call_id,
@@ -1314,33 +1311,39 @@
/* Add session name (none) */
MSGB_PRINTF_OR_RET("s=-\r\n");
- /* Add RTP address and port */
- if (mgcp_msg->audio_port == 0) {
- LOGPMGW(mgcp, LOGL_ERROR,
- "Invalid port number, can not generate MGCP message\n");
- msgb_free(msg);
- return -EINVAL;
+ /* Add RTP address */
+ if (mgcp_msg->presence & MGCP_MSG_PRESENCE_AUDIO_IP) {
+ audio_ip_family = osmo_ip_str_type(mgcp_msg->audio_ip);
+ if (audio_ip_family == AF_UNSPEC)
+ return -EINVAL;
+ if (strlen(mgcp_msg->audio_ip) <= 0) {
+ LOGPMGW(mgcp, LOGL_ERROR,
+ "Empty ip address, can not generate MGCP
message\n");
+ return -EINVAL;
+ }
+ MSGB_PRINTF_OR_RET("c=IN IP%c %s\r\n",
+ audio_ip_family == AF_INET6 ? '6' : '4',
+ mgcp_msg->audio_ip);
}
- if (strlen(mgcp_msg->audio_ip) <= 0) {
- LOGPMGW(mgcp, LOGL_ERROR,
- "Empty ip address, can not generate MGCP message\n");
- msgb_free(msg);
- return -EINVAL;
- }
- MSGB_PRINTF_OR_RET("c=IN IP%c %s\r\n",
- audio_ip_family == AF_INET6 ? '6' : '4',
- mgcp_msg->audio_ip);
/* Add time description, active time (SDP) */
MSGB_PRINTF_OR_RET("t=0 0\r\n");
- MSGB_PRINTF_OR_RET("m=audio %u RTP/AVP", mgcp_msg->audio_port);
- for (i = 0; i < mgcp_msg->codecs_len; i++) {
- pt = map_codec_to_pt(mgcp_msg->ptmap, mgcp_msg->ptmap_len,
mgcp_msg->codecs[i]);
- MSGB_PRINTF_OR_RET(" %u", pt);
+ /* Add RTP address port and codecs */
+ if (mgcp_msg->presence & MGCP_MSG_PRESENCE_AUDIO_PORT) {
+ if (mgcp_msg->audio_port == 0) {
+ LOGPMGW(mgcp, LOGL_ERROR,
+ "Invalid port number, can not generate MGCP
message\n");
+ return -EINVAL;
+ }
+ MSGB_PRINTF_OR_RET("m=audio %u RTP/AVP", mgcp_msg->audio_port);
+ for (i = 0; i < mgcp_msg->codecs_len; i++) {
+ pt = map_codec_to_pt(mgcp_msg->ptmap,
mgcp_msg->ptmap_len, mgcp_msg->codecs[i]);
+ MSGB_PRINTF_OR_RET(" %u", pt);
+ }
+ MSGB_PRINTF_OR_RET("\r\n");
}
- MSGB_PRINTF_OR_RET("\r\n");
/* Add optional codec parameters (fmtp) */
if (mgcp_msg->param_present) {
@@ -1470,10 +1473,10 @@
MSGB_PRINTF_OR_RET("I: %s\r\n", mgcp_msg->conn_id);
}
- /* Using SDP makes sense when a valid IP/Port combination is specified,
+ /* Using SDP makes sense when a valid IP or Port is specified,
* if we do not know this information yet, we fall back to LCO */
if (mgcp_msg->presence & MGCP_MSG_PRESENCE_AUDIO_IP
- && mgcp_msg->presence & MGCP_MSG_PRESENCE_AUDIO_PORT)
+ || mgcp_msg->presence & MGCP_MSG_PRESENCE_AUDIO_PORT)
use_sdp = true;
/* Add local connection options (LCO) */
diff --git a/src/libosmo-mgcp-client/mgcp_client_fsm.c
b/src/libosmo-mgcp-client/mgcp_client_fsm.c
index 432ad84..218527c 100644
---
[M] Change in osmo-mgw[master]: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
Attention is currently required from: dexter, fixeria, laforge, neels. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email ) Change subject: mgcp-client: Transmit remote IP addr in CRCX if known and port=0 .. Patch Set 6: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I30165dbac5e484011d0acf46af36f105954a501d Gerrit-Change-Number: 35152 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: fixeria Gerrit-Attention: dexter Gerrit-Comment-Date: Thu, 07 Dec 2023 11:14:12 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
[M] Change in osmo-mgw[master]: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
Attention is currently required from: dexter, fixeria, laforge, neels, pespin. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email ) Change subject: mgcp-client: Transmit remote IP addr in CRCX if known and port=0 .. Patch Set 6: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I30165dbac5e484011d0acf46af36f105954a501d Gerrit-Change-Number: 35152 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: pespin Gerrit-Attention: fixeria Gerrit-Attention: dexter Gerrit-Comment-Date: Thu, 07 Dec 2023 09:45:44 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
[M] Change in osmo-mgw[master]: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
Attention is currently required from: dexter, laforge, neels.
pespin has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email )
Change subject: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
..
Patch Set 6:
(4 comments)
File src/libosmo-mgcp-client/mgcp_client.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/f9204822_a3737a43
PS3, Line 1317: ip
> it's the other way around, this function should not be freeing msg, it's
> freed by the caller.
Done
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/657b0539_29f2fd1f
PS3, Line 1323: urn -
> I mean: not return -EINVAL, instead skip the PRINTF("c=IN"). […]
What you are proposing is imho even more complex to understand and handle, and
changing the existing behavior.
The current approach is simple and expectable imho: If the caller has an IP
address and wants to share it, it will set the appropriate existing flag to say
so. If the code filling in the mgcp message finds out it needs to fill in the
address but it finds it cannot, then it returns an error to the caller because
clearly something is unexpected and wrong at the caller.
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/c4720995_7e087348
PS3, Line 1367: *
> Ack
Done
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/969fec4e_097bff4c
PS3, Line 1373: *
> Ack
Done
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I30165dbac5e484011d0acf46af36f105954a501d
Gerrit-Change-Number: 35152
Gerrit-PatchSet: 6
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Attention: neels
Gerrit-Attention: laforge
Gerrit-Attention: dexter
Gerrit-Comment-Date: Wed, 06 Dec 2023 12:00:20 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
[M] Change in osmo-mgw[master]: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
Attention is currently required from: dexter, laforge, pespin.
neels has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email )
Change subject: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
..
Patch Set 6: Code-Review+1
(3 comments)
File src/libosmo-mgcp-client/mgcp_client.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/b0323a8d_2b31bbce
PS3, Line 1323: urn -
> I'm not following you here. […]
I mean: not return -EINVAL, instead skip the PRINTF("c=IN").
>From below comment about your preference, i gather that you don't agree with
>this point. I think you're wrong, but i'm fine to live with your choice so one
>day i can say "i told you so" =)
anyway, here are my reasons, if you'd like to know.
there are different causes for not having a valid address.
this patch handles the causes differently:
- when the flag is not set, omit the addr and send the rest.
- when the flag is set, but the addr is empty, fail the entire message, do not
send anything.
i think it would be less complexity and more stable to just always omit and
send the rest, no matter what the cause is for not having a valid IP address.
that way we don't obscurely fail sending a message at all if some flag
combination is set weirdly. Why do those flags even exist, if the value itself
can indicate presence or not...
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/b1c0ef6e_008b4619
PS3, Line 1339: et
> I prefer the way it is now. […]
Done
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/dd85524f_d2d4fc6c
PS3, Line 1353: codecs
> Whichever gets first into master makes the other one rebase it, I don't
> really care and it's fine ea […]
ok, the other patch isn't ready yet until it has backwards compat, so go ahead.
would be good if a C test covers this, to make sure it stays intact.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I30165dbac5e484011d0acf46af36f105954a501d
Gerrit-Change-Number: 35152
Gerrit-PatchSet: 6
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Attention: laforge
Gerrit-Attention: pespin
Gerrit-Attention: dexter
Gerrit-Comment-Date: Wed, 06 Dec 2023 01:26:37 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: neels
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
[M] Change in osmo-mgw[master]: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
Attention is currently required from: dexter, laforge, neels. Hello Jenkins Builder, dexter, laforge, neels, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email to look at the new patch set (#6). The following approvals got outdated and were removed: Verified+1 by Jenkins Builder Change subject: mgcp-client: Transmit remote IP addr in CRCX if known and port=0 .. mgcp-client: Transmit remote IP addr in CRCX if known and port=0 A client may know the IP address during CRCX but not yet the port, or it may simply want to hint an initial IP address so that the MGW can better guess when allocating a local IP address. Related: SYS#6657 Change-Id: I30165dbac5e484011d0acf46af36f105954a501d --- M src/libosmo-mgcp-client/mgcp_client.c M src/libosmo-mgcp-client/mgcp_client_fsm.c 2 files changed, 56 insertions(+), 31 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/52/35152/6 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I30165dbac5e484011d0acf46af36f105954a501d Gerrit-Change-Number: 35152 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: dexter Gerrit-MessageType: newpatchset
[M] Change in osmo-mgw[master]: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
Attention is currently required from: dexter, laforge, neels. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email ) Change subject: mgcp-client: Transmit remote IP addr in CRCX if known and port=0 .. Patch Set 4: (1 comment) Patchset: PS4: we may need to recover some stuff from version 1 of this patch, in order to signal to the osmo-mgw already during CRCX that we want to use codec IuFP even if we don't know the port. This way MGW gains knownledge that it will be an IuUP conn and hence can act specifically (eg. alow rx of IuUP Init without knowing the remote IuUP address). -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I30165dbac5e484011d0acf46af36f105954a501d Gerrit-Change-Number: 35152 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: dexter Gerrit-Comment-Date: Mon, 04 Dec 2023 11:18:50 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
[M] Change in osmo-mgw[master]: mgcp-client: Transmit remote IP addr in CRCX if known and port=0
Attention is currently required from: dexter, laforge, neels. Hello Jenkins Builder, dexter, laforge, neels, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Code-Review+1 by laforge, Code-Review-1 by neels, Verified+1 by Jenkins Builder Change subject: mgcp-client: Transmit remote IP addr in CRCX if known and port=0 .. mgcp-client: Transmit remote IP addr in CRCX if known and port=0 A client may know the IP address during CRCX but not yet the port, or it may simply want to hint an initial IP address so that the MGW can better guess when allocating a local IP address. Related: SYS#6657 Change-Id: I30165dbac5e484011d0acf46af36f105954a501d --- M src/libosmo-mgcp-client/mgcp_client.c M src/libosmo-mgcp-client/mgcp_client_fsm.c 2 files changed, 87 insertions(+), 62 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/52/35152/4 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I30165dbac5e484011d0acf46af36f105954a501d Gerrit-Change-Number: 35152 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: dexter Gerrit-MessageType: newpatchset
