- gerrit-log - lists.osmocom.org

[M] Change in libosmocore[master]: logging: fix nul octets in log output / use osmo_strbuf
by laforge 07 Dec '23

07 Dec '23

laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35208?usp=email ) ( 2 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: logging: fix nul octets in log output / use osmo_strbuf ...................................................................... logging: fix nul octets in log output / use osmo_strbuf Patch [1] merged few weeks ago, for yet unknown reasons, sprinkles nul characters at seemingly randomly chosen log line ends. Trying to figure out why that happens, i got tired of the unreadable cruft, and decided to migrate the _output_buf() implementation to osmo_strbuf first. With osmo_strbuf in use and implementing 1:1 what the previous code did, the odd nul octets have disappeared. So the bug was caused by unreadable code. [1] 11a416827dd9f2da6b7c1db0e1e83adb1e6e5cc8 Ia7de9d88aa5ac48ec0d5c1a931a89d21c02c5433 "logging: ensure ANSI color escape is sent in same line/before newline" Related: OS#6284 Related: Ia7de9d88aa5ac48ec0d5c1a931a89d21c02c5433 Change-Id: Ib577a5e0d7450ce93ff21f37ba3262704cbf4752 --- M src/core/logging.c 1 file changed, 70 insertions(+), 84 deletions(-) Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve neels: Looks good to me, approved laforge: Looks good to me, but someone else must approve diff --git a/src/core/logging.c b/src/core/logging.c index dc19cf3..0d215f4 100644 --- a/src/core/logging.c +++ b/src/core/logging.c @@ -486,18 +486,18 @@ unsigned int level, const char *file, int line, int cont, const char *format, va_list ap) { - int ret, len = 0, offset = 0, rem = buf_len; + int ret; const char *c_subsys = NULL; + struct osmo_strbuf sb = { .buf = buf, .len = buf_len }; + + /* safety net in case of encountering errors and returning nothing */ + buf[0] = '\0'; /* are we using color */ if (target->use_color) { c_subsys = color(subsys); - if (c_subsys) { - ret = snprintf(buf + offset, rem, "%s", c_subsys); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); - } + if (c_subsys) + OSMO_STRBUF_PRINTF(sb, "%s", c_subsys); } if (!cont) { if (target->print_ext_timestamp) { @@ -506,13 +506,10 @@ struct timeval tv; osmo_gettimeofday(&tv, NULL); localtime_r(&tv.tv_sec, &tm); - ret = snprintf(buf + offset, rem, "%04d%02d%02d%02d%02d%02d%03d ", - tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, - tm.tm_hour, tm.tm_min, tm.tm_sec, - (int)(tv.tv_usec / 1000)); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); + OSMO_STRBUF_PRINTF(sb, "%04d%02d%02d%02d%02d%02d%03d ", + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, + tm.tm_hour, tm.tm_min, tm.tm_sec, + (int)(tv.tv_usec / 1000)); #endif } else if (target->print_timestamp) { time_t tm; @@ -520,97 +517,66 @@ goto err; /* Get human-readable representation of time. man ctime: we need at least 26 bytes in buf */ - if (rem < 26 || !ctime_r(&tm, buf + offset)) + if (OSMO_STRBUF_REMAIN(sb) < 26 || !ctime_r(&tm, sb.pos)) goto err; - ret = strlen(buf + offset); + ret = strnlen(sb.pos, 26); if (ret <= 0) goto err; + OSMO_STRBUF_ADDED_TAIL(sb, ret); /* Get rid of useless final '\n' added by ctime_r. We want a space instead. */ - buf[offset + ret - 1] = ' '; - OSMO_SNPRINTF_RET(ret, rem, offset, len); + OSMO_STRBUF_DROP_TAIL(sb, 1); + OSMO_STRBUF_PRINTF(sb, " "); } if (target->print_tid) { if (logging_tid == 0) logging_tid = (long int)osmo_gettid(); - ret = snprintf(buf + offset, rem, "%ld ", logging_tid); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); + OSMO_STRBUF_PRINTF(sb, "%ld ", logging_tid); } - if (target->print_category) { - ret = snprintf(buf + offset, rem, "%s%s%s%s ", - target->use_color ? level_color(level) : "", - log_category_name(subsys), - target->use_color ? OSMO_LOGCOLOR_END : "", - c_subsys ? c_subsys : ""); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); - } - if (target->print_level) { - ret = snprintf(buf + offset, rem, "%s%s%s%s ", - target->use_color ? level_color(level) : "", - log_level_str(level), - target->use_color ? OSMO_LOGCOLOR_END : "", - c_subsys ? c_subsys : ""); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); - } - if (target->print_category_hex) { - ret = snprintf(buf + offset, rem, "<%4.4x> ", subsys); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); - } + if (target->print_category) + OSMO_STRBUF_PRINTF(sb, "%s%s%s%s ", + target->use_color ? level_color(level) : "", + log_category_name(subsys), + target->use_color ? OSMO_LOGCOLOR_END : "", + c_subsys ? c_subsys : ""); + if (target->print_level) + OSMO_STRBUF_PRINTF(sb, "%s%s%s%s ", + target->use_color ? level_color(level) : "", + log_level_str(level), + target->use_color ? OSMO_LOGCOLOR_END : "", + c_subsys ? c_subsys : ""); + if (target->print_category_hex) + OSMO_STRBUF_PRINTF(sb, "<%4.4x> ", subsys); if (target->print_filename_pos == LOG_FILENAME_POS_HEADER_END) { switch (target->print_filename2) { case LOG_FILENAME_NONE: break; case LOG_FILENAME_PATH: - ret = snprintf(buf + offset, rem, "%s:%d ", file, line); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); + OSMO_STRBUF_PRINTF(sb, "%s:%d ", file, line); break; case LOG_FILENAME_BASENAME: - ret = snprintf(buf + offset, rem, "%s:%d ", const_basename(file), line); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); + OSMO_STRBUF_PRINTF(sb, "%s:%d ", const_basename(file), line); break; } } } - ret = vsnprintf(buf + offset, rem, format, ap); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); + OSMO_STRBUF_APPEND(sb, vsnprintf, format, ap); /* For LOG_FILENAME_POS_LAST, print the source file info only when the caller ended the log * message in '\n'. If so, nip the last '\n' away, insert the source file info and re-append an * '\n'. All this to allow LOGP("start..."); LOGPC("...end\n") constructs. */ if (target->print_filename_pos == LOG_FILENAME_POS_LINE_END - && offset > 0 && buf[offset - 1] == '\n') { + && sb.pos > sb.buf && *(sb.pos - 1) == '\n') { switch (target->print_filename2) { case LOG_FILENAME_NONE: break; case LOG_FILENAME_PATH: - offset--; - len--; - ret = snprintf(buf + offset, rem, " (%s:%d)\n", file, line); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); + OSMO_STRBUF_DROP_TAIL(sb, 1); + OSMO_STRBUF_PRINTF(sb, " (%s:%d)\n", file, line); break; case LOG_FILENAME_BASENAME: - offset--; - len--; - ret = snprintf(buf + offset, rem, " (%s:%d)\n", const_basename(file), line); - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); + OSMO_STRBUF_DROP_TAIL(sb, 1); + OSMO_STRBUF_PRINTF(sb, " (%s:%d)\n", const_basename(file), line); break; } } @@ -618,21 +584,15 @@ if (target->use_color && c_subsys) { /* Ensure the last color escape is sent before the newline * (to not clobber journald, which works on single-lines only) */ - if (offset > 0 && buf[offset - 1] == '\n') { - offset--; rem++; - ret = snprintf(buf + offset, rem, OSMO_LOGCOLOR_END "\n"); + if (sb.pos > sb.buf && *(sb.pos - 1) == '\n') { + OSMO_STRBUF_DROP_TAIL(sb, 1); + OSMO_STRBUF_PRINTF(sb, OSMO_LOGCOLOR_END "\n"); } else { - ret = snprintf(buf + offset, rem, OSMO_LOGCOLOR_END); + OSMO_STRBUF_PRINTF(sb, OSMO_LOGCOLOR_END); } - - if (ret < 0) - goto err; - OSMO_SNPRINTF_RET(ret, rem, offset, len); } err: - len = OSMO_MIN(buf_len - 1, len); - buf[len] = '\0'; - return len; + return OSMO_STRBUF_CHAR_COUNT(sb); } /* Format the log line for given target; use a stack buffer and call target->output */ -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35208?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: Ib577a5e0d7450ce93ff21f37ba3262704cbf4752 Gerrit-Change-Number: 35208 Gerrit-PatchSet: 3 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[M] Change in libosmocore[master]: util: add osmo_strbuf macros to manipulate the strbuf tail
by laforge 07 Dec '23

07 Dec '23

laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35207?usp=email ) Change subject: util: add osmo_strbuf macros to manipulate the strbuf tail ...................................................................... util: add osmo_strbuf macros to manipulate the strbuf tail Upcoming patch adopts osmo_strbuf in logging.c, which sometimes needs to steal and re-add trailing newline characters, and also needs to let ctime_r() write to the buffer before updating the osmo_strbuf state. Related: OS#6284 Related: Ib577a5e0d7450ce93ff21f37ba3262704cbf4752 Change-Id: I997707c328eab3ffa00a78fdb9a0a2cbe18404b4 --- M include/osmocom/core/utils.h M src/core/libosmocore.map M src/core/utils.c M tests/utils/utils_test.c M tests/utils/utils_test.ok 5 files changed, 178 insertions(+), 0 deletions(-) Approvals: pespin: Looks good to me, but someone else must approve osmith: Looks good to me, but someone else must approve laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/include/osmocom/core/utils.h b/include/osmocom/core/utils.h index 2a3670b..b6e67e2 100644 --- a/include/osmocom/core/utils.h +++ b/include/osmocom/core/utils.h @@ -284,6 +284,12 @@ /*! Return remaining space for characters and terminating nul in the given struct osmo_strbuf. */ #define OSMO_STRBUF_REMAIN(STRBUF) ((STRBUF).buf ? (STRBUF).len - ((STRBUF).pos - (STRBUF).buf) : 0) +/*! 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) + /*! 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 * that number cannot be obtained from this kind of function signature. @@ -307,6 +313,16 @@ (STRBUF).chars_needed += _sb_l; \ } while(0) +void osmo_strbuf_drop_tail(struct osmo_strbuf *sb, size_t n_chars); +/* Convenience macro. struct osmo_strbuf are typically static to a function scope. Avoid having to type '&', same as + * with all the other OSMO_STRBUF_* API. */ +#define OSMO_STRBUF_DROP_TAIL(STRBUF, N_CHARS) osmo_strbuf_drop_tail(&(STRBUF), N_CHARS) + +void osmo_strbuf_added_tail(struct osmo_strbuf *sb, size_t n_chars); +/* Convenience macro. struct osmo_strbuf are typically static to a function scope. Avoid having to type '&', same as + * with all the other OSMO_STRBUF_* API. */ +#define OSMO_STRBUF_ADDED_TAIL(STRBUF, N_CHARS) osmo_strbuf_added_tail(&(STRBUF), N_CHARS) + bool osmo_str_startswith(const char *str, const char *startswith_str); int osmo_float_str_to_int(int64_t *val, const char *str, unsigned int precision); diff --git a/src/core/libosmocore.map b/src/core/libosmocore.map index fc81650..30c4927 100644 --- a/src/core/libosmocore.map +++ b/src/core/libosmocore.map @@ -510,6 +510,8 @@ osmo_strrb_get_nth; _osmo_strrb_is_bufindex_valid; osmo_strrb_is_empty; +osmo_strbuf_drop_tail; +osmo_strbuf_added_tail; osmo_str_startswith; osmo_str_to_int; osmo_str_to_int64; diff --git a/src/core/utils.c b/src/core/utils.c index 231b65c..882eb6f 100644 --- a/src/core/utils.c +++ b/src/core/utils.c @@ -1211,6 +1211,51 @@ return (sum * 9) % 10 + '0'; } +/*! Remove up to N chars from the end of an osmo_strbuf. + * |--char-count---| - - chars_needed - - | + * |<---------drop----------| + */ +void osmo_strbuf_drop_tail(struct osmo_strbuf *sb, size_t n_chars) +{ + size_t drop_n; + if (sb->pos <= sb->buf) + return; + drop_n = OSMO_MIN(sb->chars_needed, n_chars); + sb->chars_needed -= drop_n; + /* chars_needed was reduced by n_chars, which may have been entirely behind the end of a full buffer, within the + * hypothetical chars_needed. Modify the buffer tail pos only if the buffer is not or longer full now. */ + if (sb->chars_needed >= OSMO_STRBUF_CHAR_COUNT(*sb)) + return; + sb->pos = sb->buf + sb->chars_needed; + *sb->pos = '\0'; +} + +/*! Let osmo_strbuf know that n_chars characters (excluding nul) were written to the end of the buffer. + * If sb is nonempty, the n_chars are assumed to have been written to sb->pos. If sb is still empty and pos == NULL, the + * n_chars are assumed to have been written to the start of the buffer. + * Advance sb->pos and sb->chars_needed by at most n_chars, or up to sb->len - 1. + * Ensure nul termination. */ +void osmo_strbuf_added_tail(struct osmo_strbuf *sb, size_t n_chars) +{ + /* On init of an osmo_strbuf, sb->pos == NULL, which is defined as semantically identical to pointing at the + * start of the buffer. A caller may just write to the buffer and call osmo_strbuf_added_tail(), in which case + * still pos == NULL. pos != NULL happens as soon as the first OSMO_STRBUF_*() API has acted on the strbuf. */ + if (!sb->pos) + sb->pos = sb->buf; + sb->chars_needed += n_chars; + /* first get remaining space, not counting trailing nul; but safeguard against empty buffer */ + size_t n_added = OSMO_STRBUF_REMAIN(*sb); + if (n_added) + n_added--; + /* do not add more than fit in sb->len, still ensuring nul termination */ + n_added = OSMO_MIN(n_added, n_chars); + if (n_added) + sb->pos += n_added; + /* when a strbuf is full, sb->pos may point after the final nul, so nul terminate only when pos is valid. */ + if (sb->pos < sb->buf + sb->len) + *sb->pos = '\0'; +} + /*! Compare start of a string. * This is an optimisation of 'strstr(str, startswith_str) == str' because it doesn't search through the entire string. * \param str (Longer) string to compare. diff --git a/tests/utils/utils_test.c b/tests/utils/utils_test.c index 0b7bfe4..bdeedb5 100644 --- a/tests/utils/utils_test.c +++ b/tests/utils/utils_test.c @@ -31,6 +31,7 @@ #include <errno.h> #include <limits.h> #include <inttypes.h> +#include <string.h> static void hexdump_test(void) { @@ -1306,6 +1307,50 @@ printf("%zu: %s (need=%zu)\n", sb.len, buf, sb.chars_needed); } +void strbuf_test_tail_for_buflen(size_t buflen) +{ + char buf[buflen]; + struct osmo_strbuf sb = { .buf = buf, .len = buflen }; + printf("\n%s(%zu)\n", __func__, buflen); + +#define SHOW(N) \ + printf(#N ": %s sb.chars_needed=%zu sb.pos=&sb.buf[%d]\n", \ + osmo_quote_str(buf, -1), sb.chars_needed, (int)(sb.pos - sb.buf)) + + /* shorten in steps using OSMO_STRBUF_DROP_TAIL(), removing and re-adding a trailing newline. */ + OSMO_STRBUF_PRINTF(sb, "banananana\n"); + SHOW(1); + OSMO_STRBUF_DROP_TAIL(sb, 3); + SHOW(2); + OSMO_STRBUF_PRINTF(sb, "\n"); + SHOW(3); + OSMO_STRBUF_DROP_TAIL(sb, 3); + SHOW(4); + OSMO_STRBUF_PRINTF(sb, "\n"); + SHOW(5); + + /* drop trailing newline */ + OSMO_STRBUF_DROP_TAIL(sb, 1); + SHOW(6); + + /* test writing something to the end and letting OSMO_STRBUF_ADDED_TAIL() know later */ + int n = OSMO_MIN(6, OSMO_STRBUF_REMAIN(sb)); + if (n) + memcpy(sb.pos, "bread\n", n); + OSMO_STRBUF_ADDED_TAIL(sb, 6); + SHOW(7); +} + +void strbuf_test_tail(void) +{ + strbuf_test_tail_for_buflen(64); + strbuf_test_tail_for_buflen(32); + strbuf_test_tail_for_buflen(16); + strbuf_test_tail_for_buflen(8); + strbuf_test_tail_for_buflen(4); + strbuf_test_tail_for_buflen(1); +} + static void startswith_test_str(const char *str, const char *startswith_str, bool expect_rc) { bool rc = osmo_str_startswith(str, startswith_str); @@ -2152,6 +2197,7 @@ osmo_str_tolowupper_test(); strbuf_test(); strbuf_test_nolen(); + strbuf_test_tail(); startswith_test(); name_c_impl_test(); osmo_print_n_test(); diff --git a/tests/utils/utils_test.ok b/tests/utils/utils_test.ok index 3f453e9..6f5d46b 100644 --- a/tests/utils/utils_test.ok +++ b/tests/utils/utils_test.ok @@ -470,6 +470,60 @@ more: 0001011100101010000 (need=19) 10: 000101110 (need=9) +strbuf_test_tail_for_buflen(64) +1: "banananana\n" sb.chars_needed=11 sb.pos=&sb.buf[11] +2: "bananana" sb.chars_needed=8 sb.pos=&sb.buf[8] +3: "bananana\n" sb.chars_needed=9 sb.pos=&sb.buf[9] +4: "banana" sb.chars_needed=6 sb.pos=&sb.buf[6] +5: "banana\n" sb.chars_needed=7 sb.pos=&sb.buf[7] +6: "banana" sb.chars_needed=6 sb.pos=&sb.buf[6] +7: "bananabread\n" sb.chars_needed=12 sb.pos=&sb.buf[12] + +strbuf_test_tail_for_buflen(32) +1: "banananana\n" sb.chars_needed=11 sb.pos=&sb.buf[11] +2: "bananana" sb.chars_needed=8 sb.pos=&sb.buf[8] +3: "bananana\n" sb.chars_needed=9 sb.pos=&sb.buf[9] +4: "banana" sb.chars_needed=6 sb.pos=&sb.buf[6] +5: "banana\n" sb.chars_needed=7 sb.pos=&sb.buf[7] +6: "banana" sb.chars_needed=6 sb.pos=&sb.buf[6] +7: "bananabread\n" sb.chars_needed=12 sb.pos=&sb.buf[12] + +strbuf_test_tail_for_buflen(16) +1: "banananana\n" sb.chars_needed=11 sb.pos=&sb.buf[11] +2: "bananana" sb.chars_needed=8 sb.pos=&sb.buf[8] +3: "bananana\n" sb.chars_needed=9 sb.pos=&sb.buf[9] +4: "banana" sb.chars_needed=6 sb.pos=&sb.buf[6] +5: "banana\n" sb.chars_needed=7 sb.pos=&sb.buf[7] +6: "banana" sb.chars_needed=6 sb.pos=&sb.buf[6] +7: "bananabread\n" sb.chars_needed=12 sb.pos=&sb.buf[12] + +strbuf_test_tail_for_buflen(8) +1: "bananan" sb.chars_needed=11 sb.pos=&sb.buf[8] +2: "bananan" sb.chars_needed=8 sb.pos=&sb.buf[8] +3: "bananan" sb.chars_needed=9 sb.pos=&sb.buf[8] +4: "banana" sb.chars_needed=6 sb.pos=&sb.buf[6] +5: "banana\n" sb.chars_needed=7 sb.pos=&sb.buf[7] +6: "banana" sb.chars_needed=6 sb.pos=&sb.buf[6] +7: "bananab" sb.chars_needed=12 sb.pos=&sb.buf[7] + +strbuf_test_tail_for_buflen(4) +1: "ban" sb.chars_needed=11 sb.pos=&sb.buf[4] +2: "ban" sb.chars_needed=8 sb.pos=&sb.buf[4] +3: "ban" sb.chars_needed=9 sb.pos=&sb.buf[4] +4: "ban" sb.chars_needed=6 sb.pos=&sb.buf[4] +5: "ban" sb.chars_needed=7 sb.pos=&sb.buf[4] +6: "ban" sb.chars_needed=6 sb.pos=&sb.buf[4] +7: "ban" sb.chars_needed=12 sb.pos=&sb.buf[4] + +strbuf_test_tail_for_buflen(1) +1: "" sb.chars_needed=11 sb.pos=&sb.buf[1] +2: "" sb.chars_needed=8 sb.pos=&sb.buf[1] +3: "" sb.chars_needed=9 sb.pos=&sb.buf[1] +4: "" sb.chars_needed=6 sb.pos=&sb.buf[1] +5: "" sb.chars_needed=7 sb.pos=&sb.buf[1] +6: "" sb.chars_needed=6 sb.pos=&sb.buf[1] +7: "" sb.chars_needed=12 sb.pos=&sb.buf[1] + startswith_test() osmo_str_startswith(NULL, NULL) == true osmo_str_startswith("", NULL) == true -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35207?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: I997707c328eab3ffa00a78fdb9a0a2cbe18404b4 Gerrit-Change-Number: 35207 Gerrit-PatchSet: 3 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in libosmocore[master]: util: add OSMO_STRBUF_REMAIN()
by laforge 07 Dec '23

07 Dec '23

laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35206?usp=email ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: util: add OSMO_STRBUF_REMAIN() ...................................................................... util: add OSMO_STRBUF_REMAIN() This code already exists twice, and upcoming patch will need this as well in logging.c. Add a macro to remove the code dup. Related: OS#6284 Related: Ib577a5e0d7450ce93ff21f37ba3262704cbf4752 Change-Id: I6f2991125882bff948708bbb4ae218f9f3d1e50c --- M include/osmocom/core/utils.h 1 file changed, 19 insertions(+), 2 deletions(-) Approvals: laforge: Looks good to me, approved osmith: Looks good to me, but someone else must approve Jenkins Builder: Verified diff --git a/include/osmocom/core/utils.h b/include/osmocom/core/utils.h index ee7cfa4..2a3670b 100644 --- a/include/osmocom/core/utils.h +++ b/include/osmocom/core/utils.h @@ -249,7 +249,7 @@ #define OSMO_STRBUF_APPEND(STRBUF, func, args...) do { \ if (!(STRBUF).pos) \ (STRBUF).pos = (STRBUF).buf; \ - size_t _sb_remain = (STRBUF).buf ? (STRBUF).len - ((STRBUF).pos - (STRBUF).buf) : 0; \ + size_t _sb_remain = OSMO_STRBUF_REMAIN(STRBUF); \ int _sb_l = func((STRBUF).pos, _sb_remain, ##args); \ if (_sb_l < 0 || (size_t)_sb_l > _sb_remain) \ (STRBUF).pos = (STRBUF).buf + (STRBUF).len; \ @@ -281,6 +281,9 @@ #define OSMO_STRBUF_PRINTF(STRBUF, fmt, args...) \ OSMO_STRBUF_APPEND(STRBUF, snprintf, fmt, ##args) +/*! Return remaining space for characters and terminating nul in the given struct osmo_strbuf. */ +#define OSMO_STRBUF_REMAIN(STRBUF) ((STRBUF).buf ? (STRBUF).len - ((STRBUF).pos - (STRBUF).buf) : 0) + /*! 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 * that number cannot be obtained from this kind of function signature. @@ -292,7 +295,7 @@ #define OSMO_STRBUF_APPEND_NOLEN(STRBUF, func, args...) do { \ if (!(STRBUF).pos) \ (STRBUF).pos = (STRBUF).buf; \ - size_t _sb_remain = (STRBUF).buf ? (STRBUF).len - ((STRBUF).pos - (STRBUF).buf) : 0; \ + size_t _sb_remain = OSMO_STRBUF_REMAIN(STRBUF); \ if (_sb_remain) { \ func((STRBUF).pos, _sb_remain, ##args); \ } \ -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35206?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: I6f2991125882bff948708bbb4ae218f9f3d1e50c Gerrit-Change-Number: 35206 Gerrit-PatchSet: 3 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in pysim[master]: tlv: Fix encoding of zero-valued TLVs
by laforge 07 Dec '23

07 Dec '23

laforge has submitted this change. ( https://gerrit.osmocom.org/c/pysim/+/35243?usp=email ) Change subject: tlv: Fix encoding of zero-valued TLVs ...................................................................... tlv: Fix encoding of zero-valued TLVs If a TLV was elementary (no nested IEs), and it had only a single integer content whose value is 0, we erroneously encoded that as zero-length TLV (len=0, no value part): >>> rf = pySim.euicc.RefreshFlag(decoded=0); >>> rf.to_bytes() b'' >>> rf.to_tlv() b'\x81\x00' After this change it is correct: >>> rf = pySim.euicc.RefreshFlag(decoded=0); >>> rf.to_bytes() b'\x00' >>> rf.to_tlv() b'\x81\x01\x00' Change-Id: I5f4c0555cff7df9ccfc4a56da12766d1bf89122f --- M pySim/tlv.py 1 file changed, 28 insertions(+), 1 deletion(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/pySim/tlv.py b/pySim/tlv.py index 1ecc806..c85d92b 100644 --- a/pySim/tlv.py +++ b/pySim/tlv.py @@ -87,7 +87,7 @@ def to_bytes(self) -> bytes: """Convert from internal representation to binary bytes. Store the binary result in the internal state and return it.""" - if not self.decoded: + if self.decoded == None: do = b'' elif self._construct: do = self._construct.build(self.decoded, total_len=None) -- To view, visit https://gerrit.osmocom.org/c/pysim/+/35243?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I5f4c0555cff7df9ccfc4a56da12766d1bf89122f Gerrit-Change-Number: 35243 Gerrit-PatchSet: 2 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[S] Change in pysim[master]: euicc: Fix encoding of {enable,disable,delete}_profile
by laforge 07 Dec '23

07 Dec '23

laforge has submitted this change. ( https://gerrit.osmocom.org/c/pysim/+/35244?usp=email ) Change subject: euicc: Fix encoding of {enable,disable,delete}_profile ...................................................................... euicc: Fix encoding of {enable,disable,delete}_profile The encoding was missing a "CHOICE" container and missed the fact that the refreshFlag presence is mandatory for enable+disable. Change-Id: I12e2b16b2c1b4b01dfad0d1fb485399827f25ddc --- M pySim/euicc.py 1 file changed, 27 insertions(+), 17 deletions(-) Approvals: laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/pySim/euicc.py b/pySim/euicc.py index c89d364..c74ca66 100644 --- a/pySim/euicc.py +++ b/pySim/euicc.py @@ -206,12 +206,14 @@ pass # SGP.22 Section 5.7.16:: EnableProfile -class RefreshFlag(BER_TLV_IE, tag=0x88): # FIXME +class RefreshFlag(BER_TLV_IE, tag=0x81): # FIXME _construct = Int8ub # FIXME class EnableResult(BER_TLV_IE, tag=0x80): _construct = Enum(Int8ub, ok=0, iccidOrAidNotFound=1, profileNotInDisabledState=2, disallowedByPolicy=3, wrongProfileReenabling=4, catBusy=5, undefinedError=127) -class EnableProfileReq(BER_TLV_IE, tag=0xbf31, nested=[IsdpAid, Iccid, RefreshFlag]): +class ProfileIdentifier(BER_TLV_IE, tag=0xa0, nested=[IsdpAid, Iccid]): + pass +class EnableProfileReq(BER_TLV_IE, tag=0xbf31, nested=[ProfileIdentifier, RefreshFlag]): pass class EnableProfileResp(BER_TLV_IE, tag=0xbf31, nested=[EnableResult]): pass @@ -220,7 +222,7 @@ class DisableResult(BER_TLV_IE, tag=0x80): _construct = Enum(Int8ub, ok=0, iccidOrAidNotFound=1, profileNotInEnabledState=2, disallowedByPolicy=3, catBusy=5, undefinedError=127) -class DisableProfileReq(BER_TLV_IE, tag=0xbf32, nested=[IsdpAid, Iccid, RefreshFlag]): +class DisableProfileReq(BER_TLV_IE, tag=0xbf32, nested=[ProfileIdentifier, RefreshFlag]): pass class DisableProfileResp(BER_TLV_IE, tag=0xbf32, nested=[DisableResult]): pass @@ -229,7 +231,7 @@ class DeleteResult(BER_TLV_IE, tag=0x80): _construct = Enum(Int8ub, ok=0, iccidOrAidNotFound=1, profileNotInDisabledState=2, disallowedByPolicy=3, undefinedError=127) -class DeleteProfileReq(BER_TLV_IE, tag=0xbf33, nested=[IsdpAid, Iccid]): +class DeleteProfileReq(BER_TLV_IE, tag=0xbf33, nested=[ProfileIdentifier]): pass class DeleteProfileResp(BER_TLV_IE, tag=0xbf33, nested=[DeleteResult]): pass @@ -404,13 +406,11 @@ @cmd2.with_argparser(en_prof_parser) def do_enable_profile(self, opts): """Perform an ES10c EnableProfile function.""" - ep_cmd_contents = [] if opts.isdp_aid: - ep_cmd_contents.append(IsdpAid(decoded=opts.isdp_aid)) + p_id = ProfileIdentifier(children=[IsdpAid(decoded=opts.isdp_aid)]) if opts.iccid: - ep_cmd_contents.append(Iccid(decoded=opts.iccid)) - if opts.refresh_required: - ep_cmd_contents.append(RefreshFlag()) + p_id = ProfileIdentifier(children=[Iccid(decoded=opts.iccid)]) + ep_cmd_contents = [p_id, RefreshFlag(decoded=opts.refresh_required)] ep_cmd = EnableProfileReq(children=ep_cmd_contents) ep = ADF_ISDR.store_data_tlv(self._cmd.lchan.scc, ep_cmd, EnableProfileResp) d = ep.to_dict() @@ -425,13 +425,11 @@ @cmd2.with_argparser(dis_prof_parser) def do_disable_profile(self, opts): """Perform an ES10c DisableProfile function.""" - dp_cmd_contents = [] if opts.isdp_aid: - dp_cmd_contents.append(IsdpAid(decoded=opts.isdp_aid)) + p_id = ProfileIdentifier(children=[IsdpAid(decoded=opts.isdp_aid)]) if opts.iccid: - dp_cmd_contents.append(Iccid(decoded=opts.iccid)) - if opts.refresh_required: - dp_cmd_contents.append(RefreshFlag()) + p_id = ProfileIdentifier(children=[Iccid(decoded=opts.iccid)]) + dp_cmd_contents = [p_id, RefreshFlag(decoded=opts.refresh_required)] dp_cmd = DisableProfileReq(children=dp_cmd_contents) dp = ADF_ISDR.store_data_tlv(self._cmd.lchan.scc, dp_cmd, DisableProfileResp) d = dp.to_dict() @@ -445,11 +443,11 @@ @cmd2.with_argparser(del_prof_parser) def do_delete_profile(self, opts): """Perform an ES10c DeleteProfile function.""" - dp_cmd_contents = [] if opts.isdp_aid: - dp_cmd_contents.append(IsdpAid(decoded=opts.isdp_aid)) + p_id = ProfileIdentifier(children=[IsdpAid(decoded=opts.isdp_aid)]) if opts.iccid: - dp_cmd_contents.append(Iccid(decoded=opts.iccid)) + p_id = ProfileIdentifier(children=[Iccid(decoded=opts.iccid)]) + dp_cmd_contents = [p_id] dp_cmd = DeleteProfileReq(children=dp_cmd_contents) dp = ADF_ISDR.store_data_tlv(self._cmd.lchan.scc, dp_cmd, DeleteProfileResp) d = dp.to_dict() -- To view, visit https://gerrit.osmocom.org/c/pysim/+/35244?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I12e2b16b2c1b4b01dfad0d1fb485399827f25ddc Gerrit-Change-Number: 35244 Gerrit-PatchSet: 2 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[S] Change in pysim[master]: euicc: Fix encoding of {enable,disable,delete}_profile
by laforge 07 Dec '23

07 Dec '23

laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/35244?usp=email ) Change subject: euicc: Fix encoding of {enable,disable,delete}_profile ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/pysim/+/35244?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I12e2b16b2c1b4b01dfad0d1fb485399827f25ddc Gerrit-Change-Number: 35244 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Thu, 07 Dec 2023 13:19:49 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in pysim[master]: tlv: Fix encoding of zero-valued TLVs
by laforge 07 Dec '23

07 Dec '23

laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/35243?usp=email ) Change subject: tlv: Fix encoding of zero-valued TLVs ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/pysim/+/35243?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I5f4c0555cff7df9ccfc4a56da12766d1bf89122f Gerrit-Change-Number: 35243 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Thu, 07 Dec 2023 13:19:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in libosmo-netif[master]: stream: Use new multiaddr APIs to log whole set of sock addresses
by pespin 07 Dec '23

07 Dec '23

Attention is currently required from: pespin. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/35240?usp=email ) Change subject: stream: Use new multiaddr APIs to log whole set of sock addresses ...................................................................... Patch Set 3: -Code-Review (1 comment) File src/stream_srv.c: https://gerrit.osmocom.org/c/libosmo-netif/+/35240/comment/13923546_f9143c63 PS3, Line 373: return link->sockname; > I think I will undo this change. […] Done -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/35240?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I7ae300595825836cc7d6fa07238c0c2f15d14e85 Gerrit-Change-Number: 35240 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 07 Dec 2023 13:19:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in libosmo-netif[master]: stream: Use new multiaddr APIs to log whole set of sock addresses
by pespin 07 Dec '23

07 Dec '23

Attention is currently required from: pespin. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-netif/+/35240?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Code-Review-1 by pespin, Verified-1 by Jenkins Builder Change subject: stream: Use new multiaddr APIs to log whole set of sock addresses ...................................................................... stream: Use new multiaddr APIs to log whole set of sock addresses Until now, the used (existing) APIs were only retrieving 1 local and 1 remote address, even if the socket actually handled several, as it's possible with SCTP multi-homing setups. Related: SYS#6636 Depends: libosmocore.git Change-Id I48950754ed6f61ee5ffa04a447fab8903f10acc0 Change-Id: I7ae300595825836cc7d6fa07238c0c2f15d14e85 --- M TODO-RELEASE M include/osmocom/netif/stream.h M src/stream_cli.c M src/stream_srv.c 4 files changed, 63 insertions(+), 24 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/40/35240/4 -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/35240?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I7ae300595825836cc7d6fa07238c0c2f15d14e85 Gerrit-Change-Number: 35240 Gerrit-PatchSet: 4 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[S] Change in pysim[master]: runtime: explain why we may access the card object directly
by laforge 07 Dec '23

07 Dec '23

laforge has submitted this change. ( https://gerrit.osmocom.org/c/pysim/+/35249?usp=email ) Change subject: runtime: explain why we may access the card object directly ...................................................................... runtime: explain why we may access the card object directly When we are in the constructor of RuntimeState, we may/must access the card object directly. Let's explain why, since it may not be immediately obvious. Change-Id: I01f74d5f021d46679d1c9fa83fb8753382b0f88f Related: OS#5418 --- M pySim/runtime.py 1 file changed, 20 insertions(+), 0 deletions(-) Approvals: laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/pySim/runtime.py b/pySim/runtime.py index 2447222..670babc 100644 --- a/pySim/runtime.py +++ b/pySim/runtime.py @@ -110,6 +110,12 @@ # probe for those applications for f in sorted(set(apps_profile) - set(apps_taken), key=str): try: + # we can not use the lchan provided methods select, or select_file + # since those method work on an already finished file model. At + # this point we are still in the initialization process, so it is + # no problem when we access the card object directly without caring + # about updating other states. For normal selects at runtime, the + # caller must use the lchan provided methods select or select_file! data, sw = self.card.select_adf_by_aid(f.aid) self.selected_adf = f if sw == "9000": -- To view, visit https://gerrit.osmocom.org/c/pysim/+/35249?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I01f74d5f021d46679d1c9fa83fb8753382b0f88f Gerrit-Change-Number: 35249 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

2026

2025

2024

2023

2022

gerrit-log