Attention is currently required from: jolly, laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35983?usp=email )
Change subject: osmo_io_poll: Declare local functions "static"
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Patchset:
PS4:
this one can probably be moved to the start of the list :)
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35983?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: I6ba88cd7bbd5b5ef42eb460679696f105c9158cb
Gerrit-Change-Number: 35983
Gerrit-PatchSet: 4
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Tue, 20 Feb 2024 10:28:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: daniel, jolly, laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35982?usp=email )
Change subject: osmo_io_uring: Detach msghdr from iofd before calling iofd_handle_send_completion()
......................................................................
Patch Set 4:
(3 comments)
File src/core/osmo_io_uring.c:
https://gerrit.osmocom.org/c/libosmocore/+/35982/comment/6e1903f0_dfe8c37c
PS4, Line 199: * If there is pending data to sent, iofd_uring_submit_tx will attach it again.
to send
https://gerrit.osmocom.org/c/libosmocore/+/35982/comment/cd22fe2e_01046fff
PS4, Line 201: * iofd (which in turn frees this msghdr, if still attached) and frees the msghdr, causing a double free. */
"(which in turn frees this msghdr, if still attached) and frees the msghdr".
you are repeating yourself here?
https://gerrit.osmocom.org/c/libosmocore/+/35982/comment/4f084d16_4f109c36
PS4, Line 210: }
where does the second free happen? in iofd_uring_submit_tx?
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35982?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: Ia349f73de2145fa360b20dd40deb73a8ffc71f07
Gerrit-Change-Number: 35982
Gerrit-PatchSet: 4
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 20 Feb 2024 10:27:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: jolly, laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35910?usp=email )
Change subject: osmo_io: Use poll/select to notify socket connection at osmo_io_uring.c
......................................................................
Patch Set 4:
(4 comments)
File src/core/osmo_io_uring.c:
https://gerrit.osmocom.org/c/libosmocore/+/35910/comment/62900f2c_f007201e
PS4, Line 327: if (osmo_fd_is_registered(&iofd->u.uring.connect_ofd))
Again this is expensive! Should be removed (or conditionally enabled through ifdef).
https://gerrit.osmocom.org/c/libosmocore/+/35910/comment/0b38f7db_8c1db15b
PS4, Line 341: if (osmo_fd_is_registered(&iofd->u.uring.connect_ofd))
Again this is expensive! Should be removed (or conditionally enabled through ifdef).
https://gerrit.osmocom.org/c/libosmocore/+/35910/comment/6ebeb0dc_70109e55
PS4, Line 398: if (osmo_fd_is_registered(&iofd->u.uring.connect_ofd))
Again this is expensive! Should be removed (or conditionally enabled through ifdef).
https://gerrit.osmocom.org/c/libosmocore/+/35910/comment/cd75808a_4abffff4
PS4, Line 498: /* Use a temporary osmo_fd which we can use to notify us once the connection is established
I am not sure this is correct. According to man2 connect, the way to go is:
"""
The socket is nonblocking and the connection cannot be completed immediately. (UNIX domain sockets failed with EAGAIN instead.) It is pos‐
sible to select(2) or poll(2) for completion by selecting the socket for writing. After select(2) indicates writability, use getsockopt(2)
to read the SO_ERROR option at level SOL_SOCKET to determine whether connect() completed successfully (SO_ERROR is zero) or unsuccessfully
(SO_ERROR is one of the usual error codes listed here, explaining the reason for the failure).
"""
I don't see anywhere the mention of "fd becoming readable" as you said.
Seems you should be checking the SO_ERROR instead?
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35910?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: I4eca9ea72beb0d6ea4d44cce81ed620033f07270
Gerrit-Change-Number: 35910
Gerrit-PatchSet: 4
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Tue, 20 Feb 2024 10:19:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: daniel, jolly.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35909?usp=email )
Change subject: osmo_io: Move notify_connected function to backend
......................................................................
Patch Set 2: Code-Review-1
(2 comments)
File src/core/osmo_io_poll.c:
https://gerrit.osmocom.org/c/libosmocore/+/35909/comment/8648f276_ad8ac1e3
PS2, Line 115: if (osmo_fd_is_registered(ofd))
This is expensive!!!
See how osmo_fd_register() doesn't call osmo_fd_is_registered unless "--enable-ofd-check" is passed for debugging purposes.
https://gerrit.osmocom.org/c/libosmocore/+/35909/comment/fc25a509_79b0d19f
PS2, Line 125: if (!osmo_fd_is_registered(ofd))
This is expensive!!!
See how osmo_fd_register() doesn't call osmo_fd_is_registered unless "--enable-ofd-check" is passed for debugging purposes.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35909?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: I905ec85210570aff8addadfc9603335d04eb057a
Gerrit-Change-Number: 35909
Gerrit-PatchSet: 2
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 20 Feb 2024 10:07:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: fixeria, laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-sccp/+/35796?usp=email )
Change subject: Implement M3UA-over-TCP (in addition to SCTP)
......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
> Regarding changes v6..v7: […]
If you didn't do that because then it would signal a deprecatio nwarning, you can add OSMO_DEPRECATED_OUTSIDE_LIBOSMOSCCP, as done in osmo-mgw.git 8e8d59ff0e6f5ea1ec15473a55e79f03a532e58c
--
To view, visit https://gerrit.osmocom.org/c/libosmo-sccp/+/35796?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-Change-Id: I8c76d271472befacbeb998a93bbdc9e8660d9b5d
Gerrit-Change-Number: 35796
Gerrit-PatchSet: 7
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 20 Feb 2024 10:03:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria, laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-sccp/+/35796?usp=email )
Change subject: Implement M3UA-over-TCP (in addition to SCTP)
......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
Regarding changes v6..v7:
IMHO you are making the find_or_create2 API more complex simply to accomodate a new internal use. You can simply use the old API find_or_create there for those purposes, and avoid overloading the new API with this -1.
I don't think any new code using the library should be using this -1 feature.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-sccp/+/35796?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-Change-Id: I8c76d271472befacbeb998a93bbdc9e8660d9b5d
Gerrit-Change-Number: 35796
Gerrit-PatchSet: 7
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 20 Feb 2024 10:01:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: matanp.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/36019?usp=email )
Change subject: ctrl: Add lchan show
......................................................................
Patch Set 3:
(1 comment)
File src/osmo-bsc/bts_trx_ts_lchan_ctrl.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/36019/comment/8b8e12d0_34fa92c2
PS1, Line 84: interference = talloc_asprintf(cmd, "%ddBm(%u)", lchan->interf_dbm, lchan->interf_band);
I don't see the point of adding the units here, plus parenthesis etc. making parsing more complex.
Simply do "%d,%u", and if lchan->interf_dbm == INTERF_DBM_UNKNOWN, then use ",,"
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/36019?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I5432800eae452b6df71a151a7649f228704ed0da
Gerrit-Change-Number: 36019
Gerrit-PatchSet: 3
Gerrit-Owner: matanp <matan1008(a)gmail.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: matanp <matan1008(a)gmail.com>
Gerrit-Comment-Date: Tue, 20 Feb 2024 09:55:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment