fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/21118 )
Change subject: Configure E1 pcap file per line ...................................................................... Patch Set 7: Code-Review-1 (5 comments) CR-1 due to fd leak. https://gerrit.osmocom.org/c/libosmo-abis/+/21118/7/include/osmocom/abis/e1_input.h File include/osmocom/abis/e1_input.h: https://gerrit.osmocom.org/c/libosmo-abis/+/21118/7/include/osmocom/abis/e1_input.h@324 PS7, Line 324: e1_set_pcap_fd Should we deprecate the old symbol now? https://gerrit.osmocom.org/c/libosmo-abis/+/21118/7/src/e1_input.c File src/e1_input.c: https://gerrit.osmocom.org/c/libosmo-abis/+/21118/7/src/e1_input.c@144 PS7, Line 144: pcap_hdr It can also be 'static' I think, so it will definitely end up in RODATA. https://gerrit.osmocom.org/c/libosmo-abis/+/21118/7/src/e1_input_vty.c File src/e1_input_vty.c: https://gerrit.osmocom.org/c/libosmo-abis/+/21118/7/src/e1_input_vty.c@273 PS7, Line 273: return CMD_WARNING; I think you need to close(fd) before you return from here. https://gerrit.osmocom.org/c/libosmo-abis/+/21118/7/src/e1_input_vty.c@275 PS7, Line 275: if (line->pcap_file) { osmo_talloc_replace_string() will free() it automatically, remove this block. https://gerrit.osmocom.org/c/libosmo-abis/+/21118/7/src/e1_input_vty.c@295 PS7, Line 295: void This pointer cast is not needed because it's not 'const'. -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/21118 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: master Gerrit-Change-Id: I316c3d6a839e84c2f52a148c6b8dd6f5933cf4bf Gerrit-Change-Number: 21118 Gerrit-PatchSet: 7 Gerrit-Owner: keith <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: keith <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Comment-Date: Tue, 13 Apr 2021 19:30:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
