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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Comment-Date: Mon, 13 Mar 2023 00:05:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: fixeria <[email protected]> Gerrit-MessageType: comment
