fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32247 )
Change subject: si2quater: bts_uarfcn_add(): check if already added first
......................................................................
si2quater: bts_uarfcn_add(): check if already added first
This way we print the proper message if the given UARFCN is already
added, no matter if the UTRAN neighbour list is full or not.
Change-Id: Ife83023f6a9e28d77e44e4757457d4d1c879e78f
Related: SYS#6401
(cherry picked from commit a60d74ba9c660612833b2634435ce4f0714c7b34)
---
M src/osmo-bsc/system_information.c
1 file changed, 17 insertions(+), 3 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/47/32247/1
diff --git a/src/osmo-bsc/system_information.c b/src/osmo-bsc/system_information.c
index cfc9a04..7687c90 100644
--- a/src/osmo-bsc/system_information.c
+++ b/src/osmo-bsc/system_information.c
@@ -302,12 +302,12 @@
*ual = bts->si_common.data.uarfcn_list,
*scl = bts->si_common.data.scramble_list;
- if (len == MAX_EARFCN_LIST)
- return -ENOMEM;
-
if (pos >= 0)
return -EADDRINUSE;
+ if (len == MAX_EARFCN_LIST)
+ return -ENOMEM;
+
/* find the suitable position for arfcn if any */
pos = uarfcn_sc_pos(bts, arfcn, SC_BOUND);
i = (pos < 0) ? len : pos;
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32247
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: 2023q1
Gerrit-Change-Id: Ife83023f6a9e28d77e44e4757457d4d1c879e78f
Gerrit-Change-Number: 32247
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: laforge, pespin, fixeria.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/32043 )
Change subject: logging: add 'logging timezone (localtime|utc)'
......................................................................
Patch Set 2:
(1 comment)
File src/core/logging.c:
https://gerrit.osmocom.org/c/libosmocore/+/32043/comment/88badc79_c05ecb14
PS1, Line 874: target->timezone = timezone;
Maybe it's a good thing to not add this API at all? Being able to have a regression test for logging that is a central interface towards human users for *all* of our osmo programs is a good reason to go the extra mile. Then again if it just works, then we need no regression test, and no added timezone API.
what do you think?
If we decide to keep this, this is my response:
I made my argument and seems you disagree, but apparenty you completely ignore my arguments and make me restate them:
Yes, we could add additional validation during log_set_timestamp() and during cfg file reading, if there is a good reason. But we do not *need* to care whether a given timezone function is available. If the system cannot generate a timestamp the way it is configured, then we just don't print one in the logs. No need to abort the program or to fail a cfg file: the program can work safely either way. That's a single #if HAVE_LOCALTIME in one place, and that's it. It is *far* less complex code than what you propose:
> I don't really understand why you are even thinking about asserting or removing timestamps during logging when you can simply have a VTY command fail to set a given timestamp format when you try to set it, as per what I proposed
We don't need to be strict because the program can still run fine.
That is also how the other log_set_foo() functions seem to work.
Code dup: the reason why a given timezone may not be working is hidden in logging.c's log string generation code. I do not want to duplicate that to the function that sets the flag. When the logging output code changes, we will forget to also change the flag setting function accordingly.
So in summary, you want me to
- introduce code dup to
- add non-conforming API behavior, just to
- make it harder to launch an osmo program on a platform with no localtime_r() function.
I say, let's have simpler code, simpler starting of the program, simpler future code maintenance. The user will notice when there are no timestamps, or will just not care. either way is fine.
In our code we have lots of blocks like below,
and then one of them suddenly needs its rc checked?
log_set_print_foo(1);
log_set_this(1);
log_set_that(1);
if (log_set_timestamp(xx)) {
exit(1);
}
We can do this for a very good reason, but IMHO there just is no good reason.
Finally, this patch is so unimportant compared to other things, so i don't feel like expanding this discussion needlessly, but here we are.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32043
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I7f868b47bf8f8dfcf85e735f490ae69b18111af4
Gerrit-Change-Number: 32043
Gerrit-PatchSet: 2
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 06 Apr 2023 23:19:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32151 )
Change subject: SCCP N-PCSTATE: trigger MSC status on PC availability
......................................................................
Patch Set 3:
(1 comment)
File src/osmo-bsc/osmo_bsc_sigtran.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/32151/comment/6669a1ab_62a0a1dd
PS3, Line 224: connected = false;
> I find all this pretty confusing. […]
I added a comment that hopefully explains why this looks this way.
so yes, connected == disconnected == true can happen, and it means --> disconnected.
It's going to look confusing no matter how you implement it, because it is confusing. if you show me some code that is this but looks nicer, i'll take it.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32151
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I3a0869598b8395601a16d78dbc46eec400c0ea84
Gerrit-Change-Number: 32151
Gerrit-PatchSet: 3
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 06 Apr 2023 22:31:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment