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 <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 13 Mar 2023 00:05:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmocom-bb/+/31863 )
Change subject: layer23: fix parsing of command line options
......................................................................
layer23: fix parsing of command line options
After the recent refactoring, parsing of the command line options is
broken for some arguments. Specifically, the value of '-a'/'--arfcn'
is ignored and hard-coded ARFCN=871 is used instead.
The problem is that l23_app_init(), which allocates an MS state and
sets the initial ARFCN, is called *before* handle_options(). So the
cfg_test_arfcn is used before it gets overwritten from the argv[].
The usual approach in osmo-* apps is to parse the command line
arguments first, and only then execute code which depends on
configurable parameters. Let's follow this approach too.
Change-Id: I77ca11c14561fa3fcb9add60ccea5b0b847a20c4
---
M src/host/layer23/src/common/main.c
M src/host/layer23/src/mobile/main.c
2 files changed, 29 insertions(+), 8 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/63/31863/1
diff --git a/src/host/layer23/src/common/main.c b/src/host/layer23/src/common/main.c
index 31e7154..e4a15ee 100644
--- a/src/host/layer23/src/common/main.c
+++ b/src/host/layer23/src/common/main.c
@@ -254,14 +254,14 @@
print_copyright();
+ handle_options(argc, argv);
+
rc = l23_app_init();
if (rc < 0) {
fprintf(stderr, "Failed during l23_app_init()\n");
exit(1);
}
- handle_options(argc, argv);
-
if (l23_app_info.opt_supported & L23_OPT_VTY) {
if (_vty_init() < 0)
exit(1);
diff --git a/src/host/layer23/src/mobile/main.c b/src/host/layer23/src/mobile/main.c
index 3260bff..d24a802 100644
--- a/src/host/layer23/src/mobile/main.c
+++ b/src/host/layer23/src/mobile/main.c
@@ -255,6 +255,12 @@
print_copyright();
+ rc = handle_options(argc, argv);
+ if (rc) { /* Abort in case of parsing errors */
+ fprintf(stderr, "Error in command line options. Exiting.\n");
+ return 1;
+ }
+
srand(time(NULL));
INIT_LLIST_HEAD(&ms_list);
@@ -272,12 +278,6 @@
exit(1);
}
- rc = handle_options(argc, argv);
- if (rc) { /* Abort in case of parsing errors */
- fprintf(stderr, "Error in command line options. Exiting.\n");
- return 1;
- }
-
if (custom_cfg_file) {
/* Use full path provided by user */
config_file = talloc_strdup(l23_ctx, custom_cfg_file);
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31863
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I77ca11c14561fa3fcb9add60ccea5b0b847a20c4
Gerrit-Change-Number: 31863
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: fixeria.
Hello Jenkins Builder, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmo-netif/+/31775
to look at the new patch set (#2).
Change subject: {utils,tests}/Makefile.am: reorder libraries in LDADD
......................................................................
{utils,tests}/Makefile.am: reorder libraries in LDADD
Otherwise the linker may pick system-installed libs instead.
Change-Id: Ia639b1c5460ad9391d2c311b4978ca9374789f7a
---
M tests/Makefile.am
M utils/Makefile.am
2 files changed, 19 insertions(+), 17 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/75/31775/2
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/31775
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Ia639b1c5460ad9391d2c311b4978ca9374789f7a
Gerrit-Change-Number: 31775
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newpatchset