Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/10185 )
Change subject: port arfcn range encode support from osmo-bsc ...................................................................... Patch Set 5: Code-Review-1 (7 comments) found some hither nor thither with logging ... either set log_set_category_filter(osmo_stderr_target, DLGLOBAL, 1, LOGL_DEBUG); and also see the debug logging from the implementation, or remove logging from the regression test entirely. (no DMAIN needed) Also rather printf to stdout only. I know it is just copying from osmo-bsc, but still... here the stuff I wrote before... https://gerrit.osmocom.org/#/c/10185/5/src/gsm/gsm48_arfcn_range_encode.c File src/gsm/gsm48_arfcn_range_encode.c: https://gerrit.osmocom.org/#/c/10185/5/src/gsm/gsm48_arfcn_range_encode.c@26 PS5, Line 26: #include <osmocom/core/logging.h> looks like this file has no logging. That's a good thing. We don't need this header? https://gerrit.osmocom.org/#/c/10185/5/src/gsm/gsm48_arfcn_range_encode.c@213 PS5, Line 213: k, wk_len, octet_offs, bit_offs, level, lvl_left); ah damn. I think in a libosmocore utility function like this we shouldn't log though? not sure... https://gerrit.osmocom.org/#/c/10185/5/tests/gsm0408/gsm0408_test.c File tests/gsm0408/gsm0408_test.c: https://gerrit.osmocom.org/#/c/10185/5/tests/gsm0408/gsm0408_test.c@618 PS5, Line 618: int arfcns_num, int silent) (weird indent .. but this is just copying, right?) https://gerrit.osmocom.org/#/c/10185/5/tests/gsm0408/gsm0408_test.c@646 PS5, Line 646: range, arfcns_used, f0, f0_included); rather printf()? https://gerrit.osmocom.org/#/c/10185/5/tests/gsm0408/gsm0408_test.c@702 PS5, Line 702: fprintf(stderr, " w = "); printf https://gerrit.osmocom.org/#/c/10185/5/tests/gsm0408/gsm0408_test.c@705 PS5, Line 705: fprintf(stderr, "\n"); ... https://gerrit.osmocom.org/#/c/10185/5/tests/gsm0408/gsm0408_test.c@941 PS5, Line 941: log_init(&log_info, NULL); Usually in regression tests, to get reproducable output I have to call: log_set_use_color(osmo_stderr_target, 0); log_set_print_timestamp(osmo_stderr_target, 0); log_set_print_filename(osmo_stderr_target, 0); so how does it work that you get reproducable output on stderr? Is that even DMAIN logging there? If there is no logging happening, we could either not init logging at all, or have empty categories[]. You could also decide to ignore stderr; if this is from fprintf(stderr,..) then maybe rather printf() to stdout -- To view, visit https://gerrit.osmocom.org/10185 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia220764fba451be5e975ae7c5eefb1a25ac2bf2c Gerrit-Change-Number: 10185 Gerrit-PatchSet: 5 Gerrit-Owner: Stefan Sperling <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Stefan Sperling <[email protected]> Gerrit-CC: Holger Freyther <[email protected]> Gerrit-Comment-Date: Tue, 11 Dec 2018 11:35:01 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
