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 <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)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 <laforge(a)osmocom.org>
Gerrit-MessageType: comment