Attention is currently required from: arehbein. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33193 ) Change subject: Add osmo_io support to osmo_stream_cli and osmo_stream_srv ...................................................................... Patch Set 1: (9 comments) File src/stream.c: https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/5f97c6e3_7e6a79ed PS1, Line 367: ; I'd go for EINVAL here. It's not like there are any other modes in the enum OSMO_STREAM_MODE which we don't support. Rather, we are sure that whatever other value is passed here is invalid. https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/837aecd4_9a4229df PS1, Line 602: done I'd usually say that it is "established" after connect succeeds? https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/7974abef_299a1f21 PS1, Line 654: error %d to send sounds a bit like we are "about to send ... as an error". "received error %d in response to send" (if that's the meaning intended)? https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/3667996d_5e66d534 PS1, Line 683: osmo_iofd_setup osmo_iofd_setup may return NULL. missing error handling. https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/494069be_ebecd497 PS1, Line 1715: LOGL_DEBUG this message might be worth INFO or even NOTICE, I think. https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/240c62dd_103e83c8 PS1, Line 1758: OSMO_ASSERT why is this commented out? Is the idea to permit a srv_link in ofd mode and the stream_srv in osmo_io mode? It would be an exotic combination, but if it's technically possible, then ok. Either way the assret should either be active or removed. https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/49216b48_35b989f9 PS1, Line 1762: LOGP(DLINP, LOGL_ER if the memory allocator fails, it's always an ENOMEM situation, so it's not realyl worth calling strerror or printing something about the reason. Also, we're typically not adding log messages in such situations as otherwise we would have to write a gadzillion of them every 5th line of code. If one wanted to log this, it would be best done inside talloc or in a wrapper around talloc. That way it's one log statement for all situations. https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/40b26724_3f3f5472 PS1, Line 1768: osmo_iofd_setup osmo_iofd_setup may return NULL. Missing error handling https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/61994d55_f478f59e PS1, Line 1775: talloc_free is there nothing we need to call to undo whatever osmo_iofd_setup() has done above? -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33193 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I2f52c7107c392b6f4b0bf2a84f8c873c084a200c Gerrit-Change-Number: 33193 Gerrit-PatchSet: 1 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: daniel <dwillmann(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Comment-Date: Wed, 07 Jun 2023 09:29:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment