Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/35474?usp=email
to look at the new patch set (#2).
Change subject: utils: fix -Wsign-compare in definition of OSMO_STRBUF_CHAR_COUNT
......................................................................
utils: fix -Wsign-compare in definition of OSMO_STRBUF_CHAR_COUNT
We're seeing tons of -Wsign-compare warnings since I merged 0f59cebf:
include/osmocom/core/utils.h: In function 'size_t _osmo_strbuf_char_count(const osmo_strbuf*)':
include/osmocom/core/utils.h:24:29: error: comparison of integer expressions of different
signedness: 'long int' and 'long unsigned int'
[-Werror=sign-compare]
24 | #define OSMO_MIN(a, b) ((a) >= (b) ? (b) : (a))
| ~~~~^~~~~~
include/osmocom/core/utils.h:309:16: note: in expansion of macro 'OSMO_MIN'
309 | return OSMO_MIN(sb->pos - sb->buf, sb->len - 1);
| ^~~~~~~~
Interestingly enough, this -Wsign-compare problem has always been the
case, even before commit 0f59cebf. And somehow this did not show up
when building libosmocore.git, but only when building C++ projects
(osmo-pcu and osmo-trx).
Perhaps it has something to do with how g++ compiles extern "C" code.
Change-Id: I8e396459409e4260b8715f9e890e8972d4609a31
Fixes: 0f59cebf "utils: improve readability of OSMO_STRBUF_CHAR_COUNT"
---
M include/osmocom/core/utils.h
1 file changed, 30 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/74/35474/2
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35474?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I8e396459409e4260b8715f9e890e8972d4609a31
Gerrit-Change-Number: 35474
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-MessageType: newpatchset
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/35474?usp=email )
Change subject: utils: fix -Wsign-compare in definition of OSMO_STRBUF_CHAR_COUNT
......................................................................
utils: fix -Wsign-compare in definition of OSMO_STRBUF_CHAR_COUNT
Recent commit 0f59cebf had a problem with passing signed and unsigned
values to OSMO_MIN, what caused tons of -Wsign-compare warnings being
printed for each *.c file including <osmocom/core/utils.h>.
include/osmocom/core/utils.h: In function 'size_t _osmo_strbuf_char_count(const osmo_strbuf*)':
include/osmocom/core/utils.h:24:29: error: comparison of integer expressions of different
signedness: 'long int' and 'long unsigned int'
[-Werror=sign-compare]
24 | #define OSMO_MIN(a, b) ((a) >= (b) ? (b) : (a))
| ~~~~^~~~~~
include/osmocom/core/utils.h:309:16: note: in expansion of macro 'OSMO_MIN'
309 | return OSMO_MIN(sb->pos - sb->buf, sb->len - 1);
| ^~~~~~~~
Interestingly enough, this has always been the case before 0f59cebf.
And somehow this did not show up when building libosmocore.git,
but only for projects written in C++ (osmo-pcu and osmo-trx).
Perhaps it has something to do with how g++ compules extern "C" code.
Change-Id: I8e396459409e4260b8715f9e890e8972d4609a31
Fixes: 0f59cebf "utils: improve readability of OSMO_STRBUF_CHAR_COUNT"
---
M include/osmocom/core/utils.h
1 file changed, 31 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/74/35474/1
diff --git a/include/osmocom/core/utils.h b/include/osmocom/core/utils.h
index 9a6e6b2..92bea59 100644
--- a/include/osmocom/core/utils.h
+++ b/include/osmocom/core/utils.h
@@ -306,7 +306,7 @@
return 0;
if (sb->pos == NULL || sb->pos <= sb->buf)
return 0;
- return OSMO_MIN(sb->pos - sb->buf, sb->len - 1);
+ return OSMO_MIN((size_t)(sb->pos - sb->buf), sb->len - 1);
}
/*! Return number of actual characters contained in struct osmo_strbuf (without terminating nul). */
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35474?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I8e396459409e4260b8715f9e890e8972d4609a31
Gerrit-Change-Number: 35474
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/35471?usp=email )
Change subject: tests/utils: do not test strbuf_example2() with buf=NULL
......................................................................
tests/utils: do not test strbuf_example2() with buf=NULL
The following can be seen when building with CC=clang:
utils/utils_test.c:1239:2: runtime error: applying non-zero offset 99 to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior utils/utils_test.c:1239:2 in
utils/utils_test.c:1241:3: runtime error: applying non-zero offset 99 to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior utils/utils_test.c:1241:3 in
utils/utils_test.c:1242:2: runtime error: applying non-zero offset 99 to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior utils/utils_test.c:1242:2 in
44. testsuite.at:274: 44. utils (testsuite.at:274): FAILED (testsuite.at:278)
This makes utils_test fail due to unexpected UBSan's output.
Even though passing NULL to the strbuf API is relatively safe, it makes
no sense and the API user should ensure that this never happens. And
so we should not be testing this case.
Change-Id: Icd2323e93ec64afc1822d48e5e1d090083edf539
---
M tests/utils/utils_test.c
M tests/utils/utils_test.ok
2 files changed, 25 insertions(+), 4 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/71/35471/1
diff --git a/tests/utils/utils_test.c b/tests/utils/utils_test.c
index 26c94dc..9ab12a1 100644
--- a/tests/utils/utils_test.c
+++ b/tests/utils/utils_test.c
@@ -1280,9 +1280,6 @@
snprintf(buf, sizeof(buf), "0x2b 0x2b 0x2b...");
printf("4: (need %d chars, had size=0) %s\n", rc, buf);
- rc = strbuf_example2(NULL, 99);
- printf("5: (need %d chars, had NULL buffer)\n", rc);
-
printf("\ncascade:\n");
rc = strbuf_cascade(buf, sizeof(buf));
printf("(need %d chars)\n%s\n", rc, buf);
diff --git a/tests/utils/utils_test.ok b/tests/utils/utils_test.ok
index c0c9560..8a66ba8 100644
--- a/tests/utils/utils_test.ok
+++ b/tests/utils/utils_test.ok
@@ -458,7 +458,6 @@
2: (need 42 chars, had size=42) T minus 10 9 8 7 6 5 4 3 2 1 ... Lift off
3: (need 42 chars, had size=42+1) T minus 10 9 8 7 6 5 4 3 2 1 ... Lift off!
4: (need 42 chars, had size=0) 0x2b 0x2b 0x2b...
-5: (need 42 chars, had NULL buffer)
cascade:
(need 134 chars)
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35471?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Icd2323e93ec64afc1822d48e5e1d090083edf539
Gerrit-Change-Number: 35471
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/35473?usp=email )
Change subject: {gsm,vty}: suppress warnings about [total_]len being set but not used
......................................................................
{gsm,vty}: suppress warnings about [total_]len being set but not used
This commit fixes the following warnings seen with CC=clang:
utils.c:376:6: warning: variable 'len' set but not used [-Wunused-but-set-variable]
int len = 0, offset = 0, ret, rem;
gsm0808_utils.c:2094:6: warning: variable 'total_len' set but not used [-Wunused-but-set-variable]
int total_len = 0;
... and finally allows to build libosmocore with --enable-werror.
Change-Id: I0040ef20ba3fc53ee7ccefc4885170f333f80566
---
M src/gsm/gsm0808_utils.c
M src/vty/utils.c
2 files changed, 21 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/73/35473/1
diff --git a/src/gsm/gsm0808_utils.c b/src/gsm/gsm0808_utils.c
index 778630d..0d6d5ad 100644
--- a/src/gsm/gsm0808_utils.c
+++ b/src/gsm/gsm0808_utils.c
@@ -2094,6 +2094,7 @@
int total_len = 0;
APPEND_STR("%s:", gsm0808_cell_id_discr_name(cid->id_discr));
APPEND_CELL_ID_U(cid->id_discr, &cid->id);
+ (void)total_len; /* suppress warnings about total_len being set but not used */
return buf;
}
diff --git a/src/vty/utils.c b/src/vty/utils.c
index a651515..0f5a34e 100644
--- a/src/vty/utils.c
+++ b/src/vty/utils.c
@@ -415,6 +415,7 @@
if (ret < 0)
goto err;
OSMO_SNPRINTF_RET(ret, rem, offset, len);
+ (void)len; /* suppress warnings about len being set but not used */
err:
str[size-1] = '\0';
return str;
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35473?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I0040ef20ba3fc53ee7ccefc4885170f333f80566
Gerrit-Change-Number: 35473
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: laforge, neels.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35466?usp=email )
Change subject: core: osmo_tdef_fsm_inst_state_chg(): allow millisecond precision
......................................................................
Patch Set 3:
This change is ready for review.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35466?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I4c4ee89e7e32e86f74cd215f5cbfa44ace5426c1
Gerrit-Change-Number: 35466
Gerrit-PatchSet: 3
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Sat, 30 Dec 2023 17:22:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/35469?usp=email )
Change subject: tests/tdef: tune logging, also match stderr
......................................................................
tests/tdef: tune logging, also match stderr
Change-Id: I7f346dfbec9e724e905d26990a978495d3a9b030
---
M tests/Makefile.am
M tests/tdef/tdef_test.c
A tests/tdef/tdef_test.err
M tests/testsuite.at
4 files changed, 46 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/69/35469/1
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 478dd48..8d510d5 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -466,6 +466,7 @@
oap/oap_client_test.ok oap/oap_client_test.err \
vty/vty_transcript_test.vty \
tdef/tdef_test.ok \
+ tdef/tdef_test.err \
tdef/tdef_test_range_64bit.ok \
tdef/tdef_vty_config_root_test.vty \
tdef/tdef_vty_config_subnode_test.vty \
@@ -676,7 +677,8 @@
gsm23236/gsm23236_test \
>$(srcdir)/gsm23236/gsm23236_test.ok
tdef/tdef_test \
- >$(srcdir)/tdef/tdef_test.ok
+ >$(srcdir)/tdef/tdef_test.ok \
+ 2>$(srcdir)/tdef/tdef_test.err
sockaddr_str/sockaddr_str_test \
>$(srcdir)/sockaddr_str/sockaddr_str_test.ok
use_count/use_count_test \
diff --git a/tests/tdef/tdef_test.c b/tests/tdef/tdef_test.c
index 13dcd01..1cc9c9c 100644
--- a/tests/tdef/tdef_test.c
+++ b/tests/tdef/tdef_test.c
@@ -469,9 +469,16 @@
osmo_init_logging2(ctx, NULL);
log_set_print_filename2(osmo_stderr_target, LOG_FILENAME_NONE);
+ log_set_print_level(osmo_stderr_target, 1);
log_set_print_category(osmo_stderr_target, 1);
+ log_set_print_category_hex(osmo_stderr_target, 0);
log_set_use_color(osmo_stderr_target, 0);
+ osmo_fsm_log_addr(false);
+ osmo_fsm_log_timeouts(true);
+
+ log_set_category_filter(osmo_stderr_target, DLGLOBAL, 1, LOGL_DEBUG);
+
OSMO_ASSERT(osmo_fsm_register(&test_tdef_fsm) == 0);
test_tdef_get(argc > 1);
diff --git a/tests/tdef/tdef_test.err b/tests/tdef/tdef_test.err
new file mode 100644
index 0000000..1a39b89
--- /dev/null
+++ b/tests/tdef/tdef_test.err
@@ -0,0 +1,25 @@
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){A}: Allocated
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){A}: State change to A (T1, 100s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){A}: State change to B (T2, 1s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){B}: State change to C (T3, 3000s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){C}: State change to D (T4, 100s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){D}: State change to E (X5, 1s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){E}: State change to F (X6, 1s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){F}: State change to G (T7, 50s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){G}: State change to H (T8, 300s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){H}: State change to I (T9, 300s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){I}: State change to J (T10, 1200s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){J}: State change to K (keeping T10, 1076.954s remaining)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){K}: State change to A (T1, 100s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){A}: State change to K (keeping T1, 76.954s remaining)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){K}: State change to A (T1, 100s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){A}: State change to L (keeping T1, 76.954s remaining)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){L}: State change to O (no timeout)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){O}: State change to L (T123, 1s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){L}: State change to O (no timeout)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){O}: State change to X (no timeout)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){X}: State change to Y (T666, 999s)
+DLGLOBAL DEBUG tdef_test(test_tdef_state_timeout){Y}: State change to Z (no timeout)
+DLGLOBAL ERROR tdef_test(test_tdef_state_timeout){Z}: transition to state B not permitted!
+DLGLOBAL ERROR tdef_test(test_tdef_state_timeout){Z}: transition to state C not permitted!
+DLGLOBAL ERROR tdef_test(test_tdef_state_timeout){Z}: transition to state D not permitted!
diff --git a/tests/testsuite.at b/tests/testsuite.at
index e88e0a8..40424e1 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -414,7 +414,8 @@
AT_SETUP([tdef])
AT_KEYWORDS([tdef])
cat $abs_srcdir/tdef/tdef_test.ok > expout
-AT_CHECK([$abs_top_builddir/tests/tdef/tdef_test], [0], [expout], [ignore])
+cat $abs_srcdir/tdef/tdef_test.err > experr
+AT_CHECK([$abs_top_builddir/tests/tdef/tdef_test], [0], [expout], [experr])
AT_CLEANUP
AT_SETUP([sockaddr_str])
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35469?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I7f346dfbec9e724e905d26990a978495d3a9b030
Gerrit-Change-Number: 35469
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange
fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35313?usp=email )
Change subject: utils: improve readability of OSMO_STRBUF_CHAR_COUNT
......................................................................
utils: improve readability of OSMO_STRBUF_CHAR_COUNT
Similarly to OSMO_STRBUF_REMAIN, let's improve the code readability
by adding a static inline function. We should generally prefer using
static inline functions over macros, unless there is something that
only the proprocessor can do.
Change-Id: I71f24b87c13fd83952029171a6993f8da5e32e5b
---
M include/osmocom/core/utils.h
1 file changed, 28 insertions(+), 4 deletions(-)
Approvals:
laforge: Looks good to me, but someone else must approve
pespin: Looks good to me, but someone else must approve
neels: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/include/osmocom/core/utils.h b/include/osmocom/core/utils.h
index 973a9d0..9a6e6b2 100644
--- a/include/osmocom/core/utils.h
+++ b/include/osmocom/core/utils.h
@@ -297,11 +297,21 @@
#define OSMO_STRBUF_REMAIN(STRBUF) \
_osmo_strbuf_remain(&(STRBUF))
+/*! Get number of actual characters (without terminating nul) in the given struct osmo_strbuf.
+ * \param[in] sb the string buffer to get the number of characters for.
+ * \returns number of actual characters (without terminating nul). */
+static inline size_t _osmo_strbuf_char_count(const struct osmo_strbuf *sb)
+{
+ if (OSMO_UNLIKELY(sb == NULL || sb->buf == NULL))
+ return 0;
+ if (sb->pos == NULL || sb->pos <= sb->buf)
+ return 0;
+ return OSMO_MIN(sb->pos - sb->buf, sb->len - 1);
+}
+
/*! Return number of actual characters contained in struct osmo_strbuf (without terminating nul). */
-#define OSMO_STRBUF_CHAR_COUNT(STRBUF) ((STRBUF).buf && ((STRBUF).pos > (STRBUF).buf) ? \
- OSMO_MIN((STRBUF).pos - (STRBUF).buf, \
- (STRBUF).len - 1) \
- : 0)
+#define OSMO_STRBUF_CHAR_COUNT(STRBUF) \
+ _osmo_strbuf_char_count(&(STRBUF))
/*! Like OSMO_STRBUF_APPEND(), but for function signatures that return the char* buffer instead of a length.
* When using this function, the final STRBUF.chars_needed may not reflect the actual number of characters needed, since
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35313?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I71f24b87c13fd83952029171a6993f8da5e32e5b
Gerrit-Change-Number: 35313
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged