March 2023 - gerrit-log - lists.osmocom.org

[S] Change in libosmo-gprs[master]: rlcmac: ignore PACKET_DOWNLINK_DUMMY_CONTROL_BLOCK
by fixeria 06 Mar '23

06 Mar '23

Attention is currently required from: neels, pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/31640 ) Change subject: rlcmac: ignore PACKET_DOWNLINK_DUMMY_CONTROL_BLOCK ...................................................................... Patch Set 1: (1 comment) File src/rlcmac/rlcmac.c: https://gerrit.osmocom.org/c/libosmo-gprs/+/31640/comment/be6e4538_d61f3595 PS1, Line 392: break; /* Ignore dummy blocks */ > (weird place to put a comment IMO, seems to become a habit of yours. […] Neels: I would appreciate if you could point me to the place in https://osmocom.org/projects/cellular-infrastructure/wiki/Coding_standards or in the kernel coding guidelines recommending against using inline comments. I believe it's fine for short comments and when there is only one operator in a block (like `return`, `break`, or `continue`). This is a matter of taste and I see no need discussing this. -- To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/31640 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-gprs Gerrit-Branch: master Gerrit-Change-Id: I6a40e2795adc71b9312d39c96b01aba9a258da42 Gerrit-Change-Number: 31640 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 03:45:29 +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

1 0

[S] Change in libosmocore[master]: gsm0502: add burst length definitions from chapter 5.2
by fixeria 06 Mar '23

06 Mar '23

Attention is currently required from: neels. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31669 ) Change subject: gsm0502: add burst length definitions from chapter 5.2 ...................................................................... Patch Set 1: (2 comments) Patchset: PS1: > (I prefer enums instead of preproc constants, because then you can indicate in function args exactly […] I see no point using enums for something that will unlikely end up in a `switch` statement. And this is not like sorts of the same thing, it's bit length values... I plan to use them as offsets and buffer sizes. File include/osmocom/gsm/gsm0502.h: https://gerrit.osmocom.org/c/libosmocore/+/31669/comment/53908de4_4997e26f PS1, Line 36: #define GSM_NBITS_NB_GMSK_PAYLOAD 2 * 58 > you should have braces around that operation, at least for preproc constants. […] You say *should*, but your CR-1 looks more like *must*? :) Can you explain why is it needed to have braces around multiplication? I would agree if this was `a + b` or `a - b`, but here I don't see what do we guard against. And no, see my comment above why not enums. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31669 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I1c38ccc2b64ba9046bb3b1221d99cc55ec493802 Gerrit-Change-Number: 31669 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 03:33:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in osmocom-bb[master]: trxcon: move l1ctl_proto.h to 'include/osmocom/bb/'
by neels 06 Mar '23

06 Mar '23

Attention is currently required from: fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/31333 ) Change subject: trxcon: move l1ctl_proto.h to 'include/osmocom/bb/' ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31333 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: I2101b20637f837038115497873bd905f95a3d0c2 Gerrit-Change-Number: 31333 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 02:41:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in libosmo-gprs[master]: rlcmac: ignore PACKET_DOWNLINK_DUMMY_CONTROL_BLOCK
by neels 06 Mar '23

06 Mar '23

Attention is currently required from: pespin, fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/31640 ) Change subject: rlcmac: ignore PACKET_DOWNLINK_DUMMY_CONTROL_BLOCK ...................................................................... Patch Set 1: Code-Review+1 (1 comment) File src/rlcmac/rlcmac.c: https://gerrit.osmocom.org/c/libosmo-gprs/+/31640/comment/5725c405_ddb60426 PS1, Line 392: break; /* Ignore dummy blocks */ > Maybe add: /* TODO: 11.2.8 Update PAGE_MODE */ or alike. Other than that fine with me. (weird place to put a comment IMO, seems to become a habit of yours. comments are good and deserve their own separate line, if nothing else then to keep future patches easier to read.) -- To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/31640 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-gprs Gerrit-Branch: master Gerrit-Change-Id: I6a40e2795adc71b9312d39c96b01aba9a258da42 Gerrit-Change-Number: 31640 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 02:35:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: gsm0502: add burst length definitions from chapter 5.2
by neels 06 Mar '23

06 Mar '23

Attention is currently required from: fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31669 ) Change subject: gsm0502: add burst length definitions from chapter 5.2 ...................................................................... Patch Set 1: Code-Review-1 (1 comment) File include/osmocom/gsm/gsm0502.h: https://gerrit.osmocom.org/c/libosmocore/+/31669/comment/4f3622ae_e662fa06 PS1, Line 36: #define GSM_NBITS_NB_GMSK_PAYLOAD 2 * 58 you should have braces around that operation, at least for preproc constants. If it was an enum, no braces required =) Same everywhere below when it has operators involved. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31669 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I1c38ccc2b64ba9046bb3b1221d99cc55ec493802 Gerrit-Change-Number: 31669 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 02:33:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: gsm0502: add burst length definitions from chapter 5.2
by neels 06 Mar '23

06 Mar '23

Attention is currently required from: fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31669 ) Change subject: gsm0502: add burst length definitions from chapter 5.2 ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: (I prefer enums instead of preproc constants, because then you can indicate in function args exactly which kind of int you expect ... i replaced a lot of plain ints with enums in the gsm code over the years, e.g. where args were just "int cause" but we have many different sets of cause values in GSM. It's unusual for enums to have calculations like that, but if it can be an enum it could serve code clarity. my twopence) -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31669 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I1c38ccc2b64ba9046bb3b1221d99cc55ec493802 Gerrit-Change-Number: 31669 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 02:30:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in osmo-bts[master]: osmo-bts-trx: check lchan against NULL in bts_model_l1sap_down()
by neels 06 Mar '23

06 Mar '23

Attention is currently required from: fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31671 ) Change subject: osmo-bts-trx: check lchan against NULL in bts_model_l1sap_down() ...................................................................... Patch Set 1: Code-Review+1 (2 comments) File src/osmo-bts-trx/l1_if.c: https://gerrit.osmocom.org/c/osmo-bts/+/31671/comment/e577e27e_fb3de52f PS1, Line 408: rc (reading this makes me think that.... https://gerrit.osmocom.org/c/osmo-bts/+/31671/comment/3c17f741_b9c8c1dd PS1, Line 423: bre .... possibly this break should also set a nonzero rc?) -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31671 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I1a815c9675eebc16640b62308499dd784fc206bd Gerrit-Change-Number: 31671 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 02:27:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in osmo-bts[master]: osmo-bts-trx: clean up bts_model_l1sap_down()
by neels 06 Mar '23

06 Mar '23

Attention is currently required from: fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31670 ) Change subject: osmo-bts-trx: clean up bts_model_l1sap_down() ...................................................................... Patch Set 1: Code-Review+1 (2 comments) Patchset: PS1: weird build error, is the master build broken? File src/osmo-bts-trx/l1_if.c: https://gerrit.osmocom.org/c/osmo-bts/+/31670/comment/75a65bea_bceb6d63 PS1, Line 403: lchan = get_lchan_by_chan_nr(trx, chan_nr); (noting to other reviewers that the next patch adds lchan validity testing here, so after that it's not just two lines that could be added to each switch case) -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31670 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I915c8a541249249e3c0b1f2eda4535e7c52db79f Gerrit-Change-Number: 31670 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 02:25:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in libosmocore[master]: Update multiaddr helper
by neels 06 Mar '23

06 Mar '23

Attention is currently required from: laforge, pespin, fixeria, msuraev. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/28993 ) Change subject: Update multiaddr helper ...................................................................... Patch Set 16: (12 comments) Patchset: PS16: In this patch and its friends there is so much complexity for a relatively trivial thing to do, and this review has been going on for such a long time, and so much review ping pong has been going on. Sorry to say it but slowly but surely i feel like i'd rather be implementing this for you instead of reviewing it any longer... File src/core/socket.c: https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/ac7bdf9f_c9c46925 PS13, Line 244: OSMO_STRBUF_PRINTF(*sb, "%s%s", oss->ip, after); > We can't use osmo_sockaddr_to_str_buf2 because it always prints port in addition to host. […] ok that's true, I missed that. but you could do struct osmo_sockaddr osa = { .u.sa = add }; (struct osmo_sockaddr is much more efficient on memory and processing than osmo_sockaddr_str -- the osmo_sockaddr_str is about converting string to sockaddr, not such a good fit for the other way) File src/core/socket.c: https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/d22d48d5_d25cde57 PS16, Line 205: osmo_strbuf there no longer is an osmo_strbuf arg (which i like =) https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/d609ba31_98366eb0 PS16, Line 216: size_t count = use_hosts ? host_count : addrs_count; > what? why do you reuse the same function with 2 sets of different parameters? this is utterly confus […] funny how you add a bool use_hosts while the var hosts itself already can serve as a bool =) oh wait, how about use_hosts = hosts ? (host_count > 0) : false or actually also what Pau said, but not so critical for a static function https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/d0e24b08_84ecc282 PS16, Line 218: after ("suffix" is a better name and a word that doesn't also mean "butt") https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/209057bf_81f97d53 PS16, Line 240: osmo_sockaddr_str no osmo_sockaddr_str plz https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/8c27864a_a03a177c PS16, Line 242: osa just use &osa below https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/9c2e110d_f5ac4042 PS16, Line 248: rc = osmo_sockaddr_str_to_sockaddr(&oss, &osap->u.sas); convert a sockaddr to string and then back??? nooooooo! osa.u.sa = addrs[i]; and you're done =) https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/ce750692_bdcfb3ed PS16, Line 258: OSMO_STRBUF_PRINTF(sb, ")"); don't you also need a closing brace if count > 1? https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/6af95c22_295229dc PS16, Line 263: helper (I am against the word "helper" in function names, all functions are helpers; instead the name should convey what the function actually does) https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/94baf516_d968cb36 PS16, Line 263: static void multiaddr_log_helper(uint8_t loglevel, const char *fmt, uint16_t port, const char **hosts, size_t host_cnt) > I see no point in having this helper, with a formatting string which then anyway implies a given ord […] yes, didn't i actually post a similar comment before? https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/f09aa30d_6a39d373 PS16, Line 267: multiaddr_snprintf(strbuf, 512, hosts, host_cnt, NULL, 0); this snprintf + LOG should be surrounded by an "if (log_check_level)" condition to skip it if this logging is disabled everywhere -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/28993 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Icef53fe4b6e51563d97a1bc48001d67679b3b6e9 Gerrit-Change-Number: 28993 Gerrit-PatchSet: 16 Gerrit-Owner: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: msuraev <msuraev(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 02:06:25 +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> Comment-In-Reply-To: msuraev <msuraev(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: Add osmo_sockaddr_to_str_buf3()
by neels 06 Mar '23

06 Mar '23

Attention is currently required from: laforge, pespin, fixeria, msuraev. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/28995 ) Change subject: Add osmo_sockaddr_to_str_buf3() ...................................................................... Patch Set 11: Code-Review-2 (2 comments) Patchset: PS11: > I'm still not following why do we need this function. […] also possible to pass in an osmo_sockaddr with the port set to zero beforehand File src/core/socket.c: https://gerrit.osmocom.org/c/libosmocore/+/28995/comment/71e90348_4b29ad79 PS11, Line 1957: OSMO_STRBUF_PRINTF(sb, "["); I strongly disagree with Pau's earlier review here! We cannot change the previous behavior of osmo_sockaddr_to_str_buf2() -- after this patch, version 2 must continue to print the square braces, also when omitting a port number. This has implications for regression tests' output testing, merging this patch likely fires up red builds all over the place. If the new function should omit braces, then version 3 must be a separate implementation, while version 2 remains unchanged. Very important, this. (My personal opinion is that always printing square braces is a good idea anyway, but that doesn't really matter much for this point) -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/28995 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I36f20701663c3c7eae7fedc6551da44800b325bf Gerrit-Change-Number: 28995 Gerrit-PatchSet: 11 Gerrit-Owner: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: dexter <pmaier(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: msuraev <msuraev(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 01:44:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0