Attention is currently required from: laforge. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33201 )
Change subject: stream: Add IPA send function/IPA-mode read to srv ...................................................................... Patch Set 5: (4 comments) File src/stream.c: https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/fddbaf17_de768f0c PS1, Line 632: SRV > why SRV? isn't it CLI_OR_INP or something like that? The idea was to name the macro after the use-case it was designed for, since in all (or at least a lot of) `*_srv_*` code, `LOGP(DLINP, ...)` is done for logging https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/c6119e9e_32c596be PS1, Line 1952: from the msgb structure's l1 and possibly l2 headers > why from the l1/l2 headers? Wouldn't it be more in-line with existign code to > use something like the […] Iirc, the agreement had been to have the IPA read callback pull the headers from the message, but preserve them in l1h/l2h to enable access to them in case upper layers still need that information. And since that information hence could thus already be expected to be there (depending on the type of message received), I decided to give the caller of this function the option to access the protocol information this way for convenience. I can of course scratch it https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/134a8705_a5c083cd PS1, Line 1959: OSMO_ASSERT > below non-ipa code has OSMO_ASSERT(conn), too. Yours not. […] Yes, because `conn` isn't used by this function before it is passed to `osmo_stream_srv_send()`, which then does the check. File src/stream.c: https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/4918c925_ddf83f80 PS5, Line 1947: /* msgb_l1(msg) is expected to be set */ > I dont really get this part. […] The idea was providing the possibility of quickly reusing the headers of a message received in IPA mode (in which case `msg->l1h` will point to the IPA header, `msg->l2h` will point to the first octet after the IPA header and `msg->data` will point to the IPA payload). If we have just created a regular message buffer with an IPA message/a complete IPA message, then we would have `msg->l1h == data` and hence also the header sitting at `msg->l1h` (although `msg->l2h` would have to be set as well in case of using `IPAC_PROTO_OSMO`, but this wasn't the use-case I had in mind, what I had in mind was the above paragraph). It was something that I added because the usecase described in the first paragraph seemed convenient when I wrote the patch. I can remove the feature using these helpers along with the helper functions themselves any time. -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33201 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I61e1fe59166c46595efe8c1f32b8f2607cb6c529 Gerrit-Change-Number: 33201 Gerrit-PatchSet: 5 Gerrit-Owner: arehbein <arehb...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-CC: laforge <lafo...@osmocom.org> Gerrit-Attention: laforge <lafo...@osmocom.org> Gerrit-Comment-Date: Fri, 30 Jun 2023 17:31:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <lafo...@osmocom.org> Gerrit-MessageType: comment
