This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Vadim Yanitskiy gerrit-no-reply at lists.osmocom.orgVadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-smlc/+/20470 )
Change subject: initial working osmo-smlc implementation
......................................................................
Patch Set 6:
(22 comments)
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c
File src/osmo-smlc/cell_locations.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c@38
PS6, Line 38: uint32_t
Do we really need to enforce 'uint32_t' here? The maximum would be 63 * 550 == 34650, so 'uint16_t' or even generic 'unsigned int' / 'unsigned short' is sufficient.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c@55
PS6, Line 55:
ws
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c@194
PS6, Line 194: DEFUN
Would be nice to attach 'when-it-applies' attributes to all configuration commands.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_conn.c
File src/osmo-smlc/lb_conn.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_conn.c@49
PS6, Line 49: if (total == 0
No need for a line break.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_conn.c@78
PS6, Line 78: use_count
Hmm, so this doesn't compile?
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c
File src/osmo-smlc/lb_peer.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@35
PS6, Line 35: lb_peer_init
I would expect to see this function defined in the end of this file, just like you do in smlc_loc_req.c.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@50
PS6, Line 50: talloc_zero
Makes sense to use regular talloc() here, because you initialize the whole structure below.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@161
PS6, Line 161: sccp_lb_down_l2_cl
Looks like we have a memleak here? If so, it makes sense to start using OTC_SELECT.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@172
PS6, Line 172: struct msgb *msg = ctx->msg
Who is responsible for free()ing this msgb? The sender of a event?
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@204
PS6, Line 204:
ws
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@247
PS6, Line 247:
ws
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@283
PS6, Line 283:
ws
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_data.c
File src/osmo-smlc/smlc_data.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_data.c@27
PS6, Line 27: smlc_ctr_description
I find it hard to read, let's use line wraps please.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c
File src/osmo-smlc/smlc_loc_req.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c@68
PS6, Line 68: smlc_loc_req_fail
Why not to use a function here? I guess compiler would most likely inline it anyway.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c@86
PS6, Line 86: OSMO_ASSERT
Looks like we need talloc_or_die() / talloc_zero_or_die() API ;)
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c@152
PS6, Line 152: smlc_loc_req = (struct smlc_loc_req){
This could be twice shorter:
smlc_loc_req->lb_conn = lb_conn;
smlc_loc_req->req = *loc_req_pdu;
I don't see a reason to overwrite the whole structure twice...
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c@175
PS6, Line 175:
ws
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c@212
PS6, Line 212:
ws
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c@294
PS6, Line 294:
ws
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_main.c
File src/osmo-smlc/smlc_main.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_main.c@179
PS6, Line 179: loglevel = LOGL_NOTICE
AFAIR, it's safe to omit this filed: libosmocore would apply LOGL_NOTICE automatically.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/tests/cell_locations.vty
File tests/cell_locations.vty:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/tests/cell_locations.vty@83
PS6, Line 83: logging level llapd notice
: logging level linp notice
: logging level lmux notice
: logging level lmi notice
: logging level lmib notice
: logging level lsms notice
This shall not be a part of the transcript test: adding new logging categories to libosmocore would make it fail. The whole block till line #106 should be removed.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/tests/osmo-smlc.cfg
File tests/osmo-smlc.cfg:
PS6:
Empty config? Why not just do:
touch $(top_builddir)/tests/osmo-smlc.cfg
in the test suite?
--
To view, visit https://gerrit.osmocom.org/c/osmo-smlc/+/20470
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-smlc
Gerrit-Branch: master
Gerrit-Change-Id: I917ba8fc51a1f1150be77ae01e12a7b16a853052
Gerrit-Change-Number: 20470
Gerrit-PatchSet: 6
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: Vadim Yanitskiy <vyanitskiy at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Sat, 10 Oct 2020 21:25:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201010/936f397b/attachment.htm>