[L] Change in libosmo-netif[master]: Add osmo_io support to osmo_stream_cli and osmo_stream_srv - gerrit-log

9 Jun 2023

Attention is currently required from: arehbein, laforge, pespin.

daniel 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 7:

(1 comment)

Patchset:

PS5: 
...
   Since we
already have osmo_stream_*_get_ofd() we should add osmo_stream_*_get_iofd(). Since they
will return pointer we could simply return NULL if it's called with the wrong mode,
but maybe an assert is better.  
 The question here would be: do we really need that API for iofd? or iofd can be handled
totally internally? 
Ok, it seems this is mostly used in the actual read_cb to get the fd to perform the read.
Other uses also use the file descriptor with osmo_sock_get_name2().
So I guess we don't need it.

...

osmo_stream_srv_create2 is called with the newly accept()ed fd so I'd still pass the
read/close callbacks in create2().  
 I'd keep setting the callbacks as separate APIs. Yes, it does 2 function calls more,
occupies 2 more lines, but if we had done this already in the past then now we
wouldn't need to be changing this osmo_stream_srv_create() API in first place... so
learning for errors = adding separate setter APIs. 
Well, it's 3 more - setting the callbacks and then calling a function that actually
enables reading from the fd.

You usually call osmo_stream_srv_create() inside of the connect_cb of osmo_strem_srv_link.
At that point the fd is already connected since it has been accept()ed and might also have
pending data.
Every osmo_stream_* where the fd is initially "inactive" has separate setter
functions for the callbacks so this choice feels deliberate to me (and I agree with it).

-- 
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: 7
Gerrit-Owner: arehbein &lt;arehbein(a)sysmocom.de&gt;
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-CC: daniel &lt;dwillmann(a)sysmocom.de&gt;
Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Attention: arehbein &lt;arehbein(a)sysmocom.de&gt;
Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Comment-Date: Fri, 09 Jun 2023 12:14:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt;
Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt;
Comment-In-Reply-To: daniel &lt;dwillmann(a)sysmocom.de&gt;
Gerrit-MessageType: comment