Attention is currently required from: dexter, neels.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email )
Change subject: mgcp-cli: Transmit remote IP addr in CRCX if known and port=0
......................................................................
Patch Set 3:
(6 comments)
File src/libosmo-mgcp-client/mgcp_client.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/af2b6608_db3feb0d
PS3, Line 1317: ip
> missing msgb_free()? […]
Ack to the missing msgb_free.
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/46e8821d_54b68d11
PS3, Line 1323: urn -
> maybe we should just omit the address, same as unset flag _PRESENCE_AUDIO_IP?
I'm not following you here. The flag is set by the caller and used here, it makes no sense to unset it in this code.
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/093ca3fd_392fe499
PS3, Line 1339: et
> same as above, maybe just skip this when port == 0
I prefer the way it is now. The caller controls what it wants to send through the presence flags, and if the user wants to sent a port, it's clear that port 0 is wrong.
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/90f97406_0c117396
PS3, Line 1353: codecs
> this conflicts with branch neels/fmtp […]
Whichever gets first into master makes the other one rebase it, I don't really care and it's fine each way.
In any case, I'm fixing customer direct problems here, so it makes no sense delaying this commit imho.
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/1c78306b_82a52e21
PS3, Line 1367: *
> indent
Ack
https://gerrit.osmocom.org/c/osmo-mgw/+/35152/comment/5f670cee_be93eb67
PS3, Line 1373: *
> .
Ack
--
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: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 01 Dec 2023 15:39:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: neels.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35180?usp=email )
Change subject: socket: Introduce API osmo_sock_multiaddr_get_name_buf()
......................................................................
Patch Set 1:
(5 comments)
File src/core/socket.c:
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/ec1feec8_ff7c2583
PS1, Line 1968: return -EBADF;
> i understand, but for the snprintf() like function signature, if at all possible you should always r […]
Ack
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/1411b96c_cca4890e
PS1, Line 1993: v6 = !!s
> assigning a value to is_v6 in an 'else' clause and using is_v6 below. […]
I only care about checking if the address is ipv6 in the case where we only have 1 IP, because in that case I need to put "[]" around it to separate it from the port number. If there's more than 1, no need, because everything is already enclosed in "()".
So by doing it here I avoid doing a lookup in a string unless it's strictly needed.
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/489652e3_5bf0d19e
PS1, Line 2005: OSMO_STRBUF_PRINTF(sb, "<need-more-bufs!>");
> (could print this above instead of adding the need_more_bufs local var, just my opinion) […]
I could write an entire novel explaining the error there, but have that token printed and looking at the code already quickly explains what's going on.
I want to print it here because at least is provides as many addresses as it can fit.
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/ad4d2b6d_0d053a6e
PS1, Line 2019: bool is_v6 = false;
> (can this code dup be collapsed with the above, maybe with a static func?)
It could, I'd need to pass the sb as a param and so on, and I'm not sure if the macros can use an sb pointer....
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/4852c8d4_24954207
PS1, Line 2052: int osmo_sock_get_name_buf(char *str, size_t str_len, int fd)
> my first thought is "yes, sounds good". […]
I prefer keeping the multiaddr prefix instead of a "2", we already use it in several places. This way code knowing only to be handling single address protocols can keep using it.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35180?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I48950754ed6f61ee5ffa04a447fab8903f10acc0
Gerrit-Change-Number: 35180
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 01 Dec 2023 15:33:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: arehbein, daniel, laforge, neels.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35079?usp=email )
Change subject: osmo_io: Remove union in struct osmo_io_ops
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
my opinion here seems to be matching neels thought afaict. I see no need to remove the union. Simply check the mode always.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35079?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I138d57843edc29000530bb7896bcb239002ecbec
Gerrit-Change-Number: 35079
Gerrit-PatchSet: 4
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 01 Dec 2023 15:12:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment