[S] Change in osmocom-bb[master]: layer23: Fix cmdline args not applied - gerrit-log

12 Mar 2023

Attention is currently required from: pespin.

fixeria has posted comments on this change. (
https://gerrit.osmocom.org/c/osmocom-bb/+/31354 )

Change subject: layer23: Fix cmdline args not applied
......................................................................

Patch Set 2: Code-Review-2

(2 comments)

Commit Message:

https://gerrit.osmocom.org/c/osmocom-bb/+/31354/comment/729130cf_69ed8c0e 
PS2, Line 10: chicken-and-egg problem
...
  The point of having an MS already existing when
parsing cmdline opts is the same as for the VTY: it allows setting stuff on the object
directly. 
Ideally an MS should be allocated whenever we find an 'ms' node in the VTY config
file. This might not be the case in case of the mobile app, but this is what we normally
do in other, more actively maintained osmo-* projects.

...
  The main problem here is that there's a init()
function, which I'd like to get called *before* any other call to the layer23app API,
so that the app has time to initialize in runtime whatever it wishes. 
Well, there is nothing that prevents the app specific code to initialize whatever it
wishes if we parse argv[] first and store the parsed values in some intermediate variables
like we already do. This way we can immediately use the values parsed from the command
line.

...
  That also includes l23_app_info() which is used during
cmd line parsing. If you move cmdline parsing before l23_app_init(), you call
l23_app_info() before l23_app_init() which looks wrong to me. 
See my other comment. I really dislike the idea of `l23_app_init()` being a function and
returning a pointer to static info. By requiring all l23 apps to define the app info as a
globally accessible const symbol we can access it at any time, even during the argv[]
parsing.

...
  But you still need to allocate the MS in some apps at
l23_app_init() time because the MS object needs to be available during VTY config parsing.

As I said above, MS objects should ideally be allocated *during* the VTY config parsing,
and definitely not before. We should not make assumptions on what the user may want from
us before parsing what he/she actually wants.

...
  In any case imho those command lines should be
deprecated at some point, and VTY commands be used instead for all apps now that we
support VTY in the layer23 common code. 
For simple apps like `ccch_scan` and `ccch_scan` we still want to accept params via the
command line. Nobody would like writing a config file just to specify an ARFCN for those,
right? For more complex apps like `mobile` and `modem` we indeed should accept *all*
parameters via the VTY config file.

Patchset:

PS2: 
...
  I already asked several times that I'd like to get
an answer regading my proposals in the earlier co […] Hey Pau,

sorry for late feedback.

I had a more detailed look at the current concept of layer23 apps, and was negatively
surprised by the overkill we have in there. Specifically functions `l23_app_info()` and
`cfg_supported()`, which basically return a pointer to the app info struct, which is
pretty much static itself.

I cleaned up / refactored the code:
https://gerrit.osmocom.org/c/osmocom-bb/+/31862

and fixed the argv[] parsing by reordering function calls:
https://gerrit.osmocom.org/c/osmocom-bb/+/31863

This patch in its current stare is not making the situation better, but rather
complicating everything even further. I think it should be abandoned, sorry.

-- 
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31354
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings

Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I285a4908401659b69dac513dd1a4b07facb88c51
Gerrit-Change-Number: 31354
Gerrit-PatchSet: 2
Gerrit-Owner: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Comment-Date: Mon, 13 Mar 2023 00:05:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt;
Comment-In-Reply-To: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-MessageType: comment