Some patches

List overview All Threads
Download

newer

older

[PATCH 1/5] log: Add...

error in Sponsored-by tag

Alexander Huemer

6 Nov 2015 6 Nov '15

8:48 p.m.

Hi, here are some patches that I produced while compiling some osmocom projects and its dependencies on a new box. Most of them are trivialities that prevent compiler warnings. The patch for openbsc though fixes a bug that prevents the build when libgtp from openggsn is not present on the system. Kind regards, -Alex

Show replies by date

Alexander Huemer

6 Nov 6 Nov

8:55 p.m.

New subject: [PATCH 0/2] libosmocore

src/stat_item.c | 2 +- utils/osmo-sim-test.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)

Alexander Huemer

8:55 p.m.

New subject: [PATCH 1/2] osmo-sim-test: add missing include

--- utils/osmo-sim-test.c | 1 + 1 file changed, 1 insertion(+) diff --git a/utils/osmo-sim-test.c b/utils/osmo-sim-test.c index 9d1b481..d6299e9 100644 --- a/utils/osmo-sim-test.c +++ b/utils/osmo-sim-test.c @@ -22,6 +22,7 @@ #include <stdlib.h> #include <errno.h> #include <string.h> +#include <arpa/inet.h> #include <osmocom/core/msgb.h> #include <osmocom/core/talloc.h> -- 2.6.2

Alexander Huemer

8:55 p.m.

New subject: [PATCH 2/2] src/stat_item.c: fix cast

--- src/stat_item.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stat_item.c b/src/stat_item.c index 0545ea0..7017523 100644 --- a/src/stat_item.c +++ b/src/stat_item.c @@ -81,7 +81,7 @@ struct osmo_stat_item_group *osmo_stat_item_group_alloc(void *ctx, size = (size + sizeof(void *) - 1) & ~(sizeof(void *) - 1); /* Store offsets into the item array */ - group->items[item_idx] = (void *)items_size; + group->items[item_idx] = (struct osmo_stat_item *)items_size; items_size += size; } -- 2.6.2

Holger Freyther

7 Nov 7 Nov

12:30 p.m.

New subject: [PATCH 2/2] src/stat_item.c: fix cast

...

On 06 Nov 2015, at 20:55, Alexander Huemer <alexander.huemer(a)xx.vu> wrote:

Hi Jacob,

...

src/stat_item.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stat_item.c b/src/stat_item.c index 0545ea0..7017523 100644 --- a/src/stat_item.c +++ b/src/stat_item.c @@ -81,7 +81,7 @@ struct osmo_stat_item_group *osmo_stat_item_group_alloc(void *ctx, size = (size + sizeof(void *) - 1) & ~(sizeof(void *) - 1); /* Store offsets into the item array */ - group->items[item_idx] = (void *)items_size; + group->items[item_idx] = (struct osmo_stat_item *)items_size;

This will silence the warning but I think we could change the code to be more direct at the cost of recalculating the offset twice but staying with the pointers of the right type?

Holger Freyther

15 Jan 15 Jan

3:03 p.m.

New subject: [PATCH 2/2] src/stat_item.c: fix cast

...

On 07 Nov 2015, at 12:30, Holger Freyther <holger(a)freyther.de> wrote:

This will silence the warning but I think we could change the code to be more direct at the cost of recalculating the offset twice but staying with the pointers of the right type?

@Jacob: ping?

Alexander Huemer

6 Nov 6 Nov

8:56 p.m.

New subject: [PATCH 0/1] libosmo-abis

include/osmocom/abis/lapd.h | 2 ++ 1 file changed, 2 insertions(+)

Alexander Huemer

8:56 p.m.

New subject: [PATCH] export lapd_tei_alloc(), used in libosmo-netif

--- include/osmocom/abis/lapd.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/osmocom/abis/lapd.h b/include/osmocom/abis/lapd.h index 2457cde..2987633 100644 --- a/include/osmocom/abis/lapd.h +++ b/include/osmocom/abis/lapd.h @@ -50,6 +50,8 @@ enum lapd_recv_errors { __LAPD_ERR_MAX }; +struct lapd_tei *lapd_tei_alloc(struct lapd_instance *li, uint8_t tei); + int lapd_receive(struct lapd_instance *li, struct msgb *msg, int *error); void lapd_transmit(struct lapd_instance *li, uint8_t tei, uint8_t sapi, -- 2.6.2

Alexander Huemer

8:57 p.m.

New subject: [PATCH 0/1] libosmo-netif

examples/lapd-over-datagram-network.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

Alexander Huemer

8:57 p.m.

New subject: [PATCH] teip is a pointer, not an int

--- examples/lapd-over-datagram-network.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/lapd-over-datagram-network.c b/examples/lapd-over-datagram-network.c index 74bba74..8d3fbde 100644 --- a/examples/lapd-over-datagram-network.c +++ b/examples/lapd-over-datagram-network.c @@ -129,7 +129,7 @@ void lapd_rx_cb(struct osmo_dlsap_prim *dp, uint8_t tei, uint8_t sapi, int main(int argc, char *argv[]) { - int teip; + struct lapd_tei *teip; tall_test = talloc_named_const(NULL, 1, "lapd_test"); @@ -159,7 +159,7 @@ int main(int argc, char *argv[]) } teip = lapd_tei_alloc(lapd, tei); - if (teip == 0) { + if (teip == NULL) { LOGP(DLAPDTEST, LOGL_ERROR, "cannot assign TEI\n"); exit(EXIT_FAILURE); } -- 2.6.2

Alexander Huemer

8:58 p.m.

New subject: [PATCH 0/1] openbsc

openbsc/tests/oap/Makefile.am | 4 ++++ 1 file changed, 4 insertions(+)

Alexander Huemer

8:58 p.m.

New subject: [PATCH] tests/oap: depend on libgtp

exclude logic copied from src/gprs/Makefile.am --- openbsc/tests/oap/Makefile.am | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/openbsc/tests/oap/Makefile.am b/openbsc/tests/oap/Makefile.am index e160902..3a76b11 100644 --- a/openbsc/tests/oap/Makefile.am +++ b/openbsc/tests/oap/Makefile.am @@ -3,7 +3,11 @@ AM_CFLAGS=-Wall -ggdb3 $(LIBOSMOCORE_CFLAGS) $(LIBOSMOGSM_CFLAGS) EXTRA_DIST = oap_test.ok +if HAVE_LIBGTP +if HAVE_LIBCARES noinst_PROGRAMS = oap_test +endif +endif oap_test_SOURCES = oap_test.c -- 2.6.2

Alexander Huemer

8:58 p.m.

New subject: [PATCH 0/3] openggsn

Makefile.am | 1 + configure.in => configure.ac | 0 gtp/gtp.c | 11 ++++++----- gtp/pdp.c | 7 ++++--- 4 files changed, 11 insertions(+), 8 deletions(-)

Alexander Huemer

8:58 p.m.

New subject: [PATCH 1/3] configure.in -> configure.ac

--- configure.in => configure.ac | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename configure.in => configure.ac (100%) diff --git a/configure.in b/configure.ac similarity index 100% rename from configure.in rename to configure.ac -- 2.6.2

Alexander Huemer

8:59 p.m.

New subject: [PATCH 2/3] Makefile.am: define ACLOCAL_AMFLAGS, include directory m4

--- Makefile.am | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile.am b/Makefile.am index 960ca07..78b83f1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1,4 +1,5 @@ ## Process this file with automake to produce Makefile.in +ACLOCAL_AMFLAGS = -I m4 SUBDIRS = lib gtp ggsn sgsnemu doc pkgconfigdir = $(libdir)/pkgconfig -- 2.6.2

Alexander Huemer

8:59 p.m.

New subject: [PATCH 3/3] fix some format specifiers

--- gtp/gtp.c | 11 ++++++----- gtp/pdp.c | 7 ++++--- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/gtp/gtp.c b/gtp/gtp.c index 4436336..a3772ff 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -47,6 +47,7 @@ #include <string.h> #include <errno.h> #include <fcntl.h> +#include <inttypes.h> #include <arpa/inet.h> @@ -2676,7 +2677,7 @@ int gtp_decaps0(struct gsn_t *gsn) return 0; gsn->err_readfrom++; LOGP(DLGTP, LOGL_ERROR, - "recvfrom(fd0=%d, buffer=%lx, len=%d) failed: status = %d error = %s\n", + "recvfrom(fd0=%d, buffer=%lx, len=%zu) failed: status = %d error = %s\n", gsn->fd0, (unsigned long)buffer, sizeof(buffer), status, status ? strerror(errno) : "No error"); return -1; @@ -2821,7 +2822,7 @@ int gtp_decaps1c(struct gsn_t *gsn) return 0; gsn->err_readfrom++; LOGP(DLGTP, LOGL_ERROR, - "recvfrom(fd=%d, buffer=%lx, len=%d) failed: status = %d error = %s\n", + "recvfrom(fd=%d, buffer=%lx, len=%zu) failed: status = %d error = %s\n", fd, (unsigned long)buffer, sizeof(buffer), status, status ? strerror(errno) : "No error"); return -1; @@ -2996,7 +2997,7 @@ int gtp_decaps1u(struct gsn_t *gsn) return 0; gsn->err_readfrom++; LOGP(DLGTP, LOGL_ERROR, - "recvfrom(fd1u=%d, buffer=%lx, len=%d) failed: status = %d error = %s\n", + "recvfrom(fd1u=%d, buffer=%lx, len=%zu) failed: status = %d error = %s\n", gsn->fd1u, (unsigned long)buffer, sizeof(buffer), status, status ? strerror(errno) : "No error"); @@ -3129,7 +3130,7 @@ int gtp_data_req(struct gsn_t *gsn, struct pdp_t *pdp, void *pack, unsigned len) if (len > sizeof(union gtp_packet) - sizeof(struct gtp0_header)) { gsn->err_memcpy++; LOGP(DLGTP, LOGL_ERROR, - "Memcpy failed: %d > %d\n", len, + "Memcpy failed: %u > %zu\n", len, sizeof(union gtp_packet) - sizeof(struct gtp0_header)); return EOF; @@ -3152,7 +3153,7 @@ int gtp_data_req(struct gsn_t *gsn, struct pdp_t *pdp, void *pack, unsigned len) sizeof(struct gtp1_header_long)) { gsn->err_memcpy++; LOGP(DLGTP, LOGL_ERROR, - "Memcpy failed: %d > %d\n", len, + "Memcpy failed: %u > %zu\n", len, sizeof(union gtp_packet) - sizeof(struct gtp0_header)); return EOF; diff --git a/gtp/pdp.c b/gtp/pdp.c index e28ffac..f0d6adf 100644 --- a/gtp/pdp.c +++ b/gtp/pdp.c @@ -26,6 +26,7 @@ #include <sys/types.h> #include <netinet/in.h> #include <string.h> +#include <inttypes.h> #include "pdp.h" #include "lookupa.h" @@ -211,7 +212,7 @@ int pdp_tidset(struct pdp_t *pdp, uint64_t tid) int hash = pdp_tidhash(tid); struct pdp_t *pdp2; struct pdp_t *pdp_prev = NULL; - DEBUGP(DLGTP, "Begin pdp_tidset tid = %llx\n", tid); + DEBUGP(DLGTP, "Begin pdp_tidset tid = %"PRIx64"\n", tid); pdp->tidnext = NULL; pdp->tid = tid; for (pdp2 = hashtid[hash]; pdp2; pdp2 = pdp2->tidnext) @@ -229,7 +230,7 @@ int pdp_tiddel(struct pdp_t *pdp) int hash = pdp_tidhash(pdp->tid); struct pdp_t *pdp2; struct pdp_t *pdp_prev = NULL; - DEBUGP(DLGTP, "Begin pdp_tiddel tid = %llx\n", pdp->tid); + DEBUGP(DLGTP, "Begin pdp_tiddel tid = %"PRIx64"\n", pdp->tid); for (pdp2 = hashtid[hash]; pdp2; pdp2 = pdp2->tidnext) { if (pdp2 == pdp) { if (!pdp_prev) @@ -249,7 +250,7 @@ int pdp_tidget(struct pdp_t **pdp, uint64_t tid) { int hash = pdp_tidhash(tid); struct pdp_t *pdp2; - DEBUGP(DLGTP, "Begin pdp_tidget tid = %llx\n", tid); + DEBUGP(DLGTP, "Begin pdp_tidget tid = %"PRIx64"\n", tid); for (pdp2 = hashtid[hash]; pdp2; pdp2 = pdp2->tidnext) { if (pdp2->tid == tid) { *pdp = pdp2; -- 2.6.2

Alexander Huemer

8:59 p.m.

New subject: [PATCH 0/2] osmo-bts

Alexander Huemer

8:59 p.m.

New subject: [PATCH 1/2] fix some format specifiers

--- src/common/msg_utils.c | 6 +++--- src/common/vty.c | 5 +++-- tests/agch/agch_test.c | 9 +++++---- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/common/msg_utils.c b/src/common/msg_utils.c index 22a3012..841ffe6 100644 --- a/src/common/msg_utils.c +++ b/src/common/msg_utils.c @@ -96,7 +96,7 @@ int msg_verify_ipa_structure(struct msgb *msg) if (msgb_l1len(msg) < sizeof(struct ipaccess_head)) { LOGP(DL1C, LOGL_ERROR, - "Ipa header insufficient space %d %d\n", + "Ipa header insufficient space %d %zu\n", msgb_l1len(msg), sizeof(struct ipaccess_head)); return -1; } @@ -105,7 +105,7 @@ int msg_verify_ipa_structure(struct msgb *msg) if (ntohs(hh->len) != msgb_l1len(msg) - sizeof(struct ipaccess_head)) { LOGP(DL1C, LOGL_ERROR, - "Incorrect ipa header msg size %d %d\n", + "Incorrect ipa header msg size %d %zu\n", ntohs(hh->len), msgb_l1len(msg) - sizeof(struct ipaccess_head)); return -1; } @@ -142,7 +142,7 @@ int msg_verify_oml_structure(struct msgb *msg) struct abis_om_hdr *omh; if (msgb_l2len(msg) < sizeof(*omh)) { - LOGP(DL1C, LOGL_ERROR, "Om header insufficient space %d %d\n", + LOGP(DL1C, LOGL_ERROR, "Om header insufficient space %d %zu\n", msgb_l2len(msg), sizeof(*omh)); return -1; } diff --git a/src/common/vty.c b/src/common/vty.c index 0d52dd7..08ad5d4 100644 --- a/src/common/vty.c +++ b/src/common/vty.c @@ -21,6 +21,7 @@ #include "btsconfig.h" +#include <inttypes.h> #include <sys/socket.h> #include <netinet/in.h> #include <arpa/inet.h> @@ -577,8 +578,8 @@ static void bts_dump_vty(struct vty *vty, struct gsm_bts *bts) paging_get_queue_max(btsb->paging_state), paging_queue_length(btsb->paging_state), paging_get_lifetime(btsb->paging_state), VTY_NEWLINE); vty_out(vty, " AGCH: Queue limit %u, occupied %d, " - "dropped %llu, merged %llu, rejected %llu, " - "ag-res %llu, non-res %llu%s", + "dropped %"PRIu64", merged %"PRIu64", rejected %"PRIu64", " + "ag-res %"PRIu64", non-res %"PRIu64"%s", btsb->agch_max_queue_length, btsb->agch_queue_length, btsb->agch_queue_dropped_msgs, btsb->agch_queue_merged_msgs, btsb->agch_queue_rejected_msgs, btsb->agch_queue_agch_msgs, diff --git a/tests/agch/agch_test.c b/tests/agch/agch_test.c index 24fa847..4175bdd 100644 --- a/tests/agch/agch_test.c +++ b/tests/agch/agch_test.c @@ -25,6 +25,7 @@ #include <osmo-bts/logging.h> #include <osmo-bts/gsm_data.h> +#include <inttypes.h> #include <unistd.h> static struct gsm_bts *bts; @@ -143,8 +144,8 @@ static void test_agch_queue(void) printf("AGCH filled: count %u, imm.ass %d, imm.ass.rej %d (refs %d), " "queue limit %u, occupied %d, " - "dropped %llu, merged %llu, rejected %llu, " - "ag-res %llu, non-res %llu\n", + "dropped %"PRIu64", merged %"PRIu64", rejected %"PRIu64", " + "ag-res %"PRIu64", non-res %"PRIu64"\n", count, imm_ass_count, imm_ass_rej_count, imm_ass_rej_ref_count, btsb->agch_max_queue_length, btsb->agch_queue_length, btsb->agch_queue_dropped_msgs, btsb->agch_queue_merged_msgs, @@ -182,8 +183,8 @@ static void test_agch_queue(void) printf("AGCH drained: multiframes %u, imm.ass %d, imm.ass.rej %d (refs %d), " "queue limit %u, occupied %d, " - "dropped %llu, merged %llu, rejected %llu, " - "ag-res %llu, non-res %llu\n", + "dropped %"PRIu64", merged %"PRIu64", rejected %"PRIu64", " + "ag-res %"PRIu64", non-res %"PRIu64"\n", multiframes, imm_ass_count, imm_ass_rej_count, imm_ass_rej_ref_count, btsb->agch_max_queue_length, btsb->agch_queue_length, btsb->agch_queue_dropped_msgs, btsb->agch_queue_merged_msgs, -- 2.6.2

Alexander Huemer

8:59 p.m.

New subject: [PATCH 2/2] add some missing includes

--- src/common/bts_ctrl_lookup.c | 1 + src/common/l1sap.c | 1 + src/common/rsl.c | 1 + 3 files changed, 3 insertions(+) diff --git a/src/common/bts_ctrl_lookup.c b/src/common/bts_ctrl_lookup.c index e693718..3857ec3 100644 --- a/src/common/bts_ctrl_lookup.c +++ b/src/common/bts_ctrl_lookup.c @@ -26,6 +26,7 @@ #include <osmocom/ctrl/ports.h> #include <osmo-bts/logging.h> #include <osmo-bts/gsm_data.h> +#include <osmo-bts/control_if.h> extern vector ctrl_node_vec; diff --git a/src/common/l1sap.c b/src/common/l1sap.c index d5dd8a6..894d659 100644 --- a/src/common/l1sap.c +++ b/src/common/l1sap.c @@ -45,6 +45,7 @@ #include <osmo-bts/rsl.h> #include <osmo-bts/bts_model.h> #include <osmo-bts/handover.h> +#include <osmo-bts/power_control.h> static int l1sap_down(struct gsm_bts_trx *trx, struct osmo_phsap_prim *l1sap); diff --git a/src/common/rsl.c b/src/common/rsl.c index a225155..f2f63f8 100644 --- a/src/common/rsl.c +++ b/src/common/rsl.c @@ -47,6 +47,7 @@ #include <osmo-bts/handover.h> #include <osmo-bts/cbch.h> #include <osmo-bts/l1sap.h> +#include <osmo-bts/bts_model.h> //#define FAKE_CIPH_MODE_COMPL -- 2.6.2

Alexander Huemer

9 p.m.

New subject: [PATCH 0/2] osmo-pcu

Alexander Huemer

9 p.m.

New subject: [PATCH 1/2] buildsystem: follow libtoolize suggestions

--- .gitignore | 2 ++ Makefile.am | 1 + configure.ac | 2 ++ m4/DUMMY | 0 4 files changed, 5 insertions(+) create mode 100644 m4/DUMMY diff --git a/.gitignore b/.gitignore index 6cc9aa5..93bd908 100644 --- a/.gitignore +++ b/.gitignore @@ -18,6 +18,8 @@ install-sh missing libtool ltmain.sh +m4/*.m4 +compile core core.* diff --git a/Makefile.am b/Makefile.am index 4cbc114..80ecaac 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1,3 +1,4 @@ +ACLOCAL_AMFLAGS = -I m4 AUTOMAKE_OPTIONS = foreign dist-bzip2 1.6 SUBDIRS = src examples tests diff --git a/configure.ac b/configure.ac index 876f7d4..eabb2e7 100644 --- a/configure.ac +++ b/configure.ac @@ -16,6 +16,8 @@ AC_PROG_CXX AC_PROG_INSTALL LT_INIT +AC_CONFIG_MACRO_DIR([m4]) + dnl checks for header files AC_HEADER_STDC diff --git a/m4/DUMMY b/m4/DUMMY new file mode 100644 index 0000000..e69de29 -- 2.6.2

Holger Freyther

7 Nov 7 Nov

12:45 p.m.

New subject: [PATCH 1/2] buildsystem: follow libtoolize suggestions

...

On 06 Nov 2015, at 21:00, Alexander Huemer <alexander.huemer(a)xx.vu> wrote:

Well, we don't have any m4 macros ourselves and just creating an empty directory doesn't look like a good idea. Have you considered asking on the libtool/autoconf list where this warning is coming from and what to do about it? thanks holger

Alexander Huemer

2:07 p.m.

New subject: [PATCH 1/2] buildsystem: follow libtoolize suggestions

Hi! On Sat, Nov 07, 2015 at 12:45:33PM +0100, Holger Freyther wrote:

...

On 06 Nov 2015, at 21:00, Alexander Huemer <alexander.huemer(a)xx.vu> wrote:

Are you sure this approach is wrong? In libosmocore the same was done in commit b2eb83fa95b209fb01de2996a1382c944fc265fe of 2010, I actually took the filename from there. If you prefer I can check with the autotools people. Kind regards, -Alex

Holger Freyther

15 Jan 15 Jan

3:37 p.m.

New subject: [PATCH 1/2] buildsystem: follow libtoolize suggestions

...

On 07 Nov 2015, at 14:07, Alexander Huemer <alexander.huemer(a)xx.vu> wrote: Hi! On Sat, Nov 07, 2015 at 12:45:33PM +0100, Holger Freyther wrote:

On 06 Nov 2015, at 21:00, Alexander Huemer <alexander.huemer(a)xx.vu> wrote:

yes, please. I mean why do we create a m4/DUMMY just to fix a suggestion by a tool? It shows that we don't have a need for this directory right now?

Alexander Huemer

4:22 p.m.

New subject: [PATCH 1/2] buildsystem: follow libtoolize suggestions

On Fri, Jan 15, 2016 at 03:37:27PM +0100, Holger Freyther wrote:

...

On 07 Nov 2015, at 14:07, Alexander Huemer <alexander.huemer(a)xx.vu> wrote: Hi! On Sat, Nov 07, 2015 at 12:45:33PM +0100, Holger Freyther wrote:

On 06 Nov 2015, at 21:00, Alexander Huemer <alexander.huemer(a)xx.vu> wrote:

yes, please. I mean why do we create a m4/DUMMY just to fix a suggestion by a tool? It shows that we don't have a need for this directory right now?

As I see the situation the approach is exactly that. Adding a file that does not hurt to make the tool happy. There are no other consequences. If you prefer to keep the repository clean of files and directories that do not add functionality, that's a of course a approach as well. BTW, I asked on the freenode#autotools channel back in November, the guys there told me the same thing. Kind regards, -Alex

Alexander Huemer

6 Nov 6 Nov

9 p.m.

New subject: [PATCH 2/2] fix some format specifiers

--- src/pcu_vty_functions.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/pcu_vty_functions.cpp b/src/pcu_vty_functions.cpp index d2a3641..df24171 100644 --- a/src/pcu_vty_functions.cpp +++ b/src/pcu_vty_functions.cpp @@ -22,6 +22,7 @@ #include <stdint.h> #include <stdlib.h> +#include <inttypes.h> #include "pcu_vty_functions.h" #include "bts.h" #include "gprs_ms_storage.h" @@ -47,7 +48,7 @@ int pcu_vty_show_ms_all(struct vty *vty, struct gprs_rlcmac_bts *bts_data) llist_for_each(ms_iter, &bts->ms_store().ms_list()) { GprsMs *ms = ms_iter->entry(); - vty_out(vty, "MS TLLI=%08x, TA=%d, CS-UL=%d, CS-DL=%d, LLC=%d, " + vty_out(vty, "MS TLLI=%08x, TA=%d, CS-UL=%d, CS-DL=%d, LLC=%zu, " "IMSI=%s%s", ms->tlli(), ms->ta(), ms->current_cs_ul(), ms->current_cs_dl(), @@ -70,9 +71,9 @@ static int show_ms(struct vty *vty, GprsMs *ms) vty_out(vty, " Coding scheme downlink: CS-%d%s", ms->current_cs_dl(), VTY_NEWLINE); vty_out(vty, " MS class: %d%s", ms->ms_class(), VTY_NEWLINE); - vty_out(vty, " LLC queue length: %d%s", ms->llc_queue()->size(), + vty_out(vty, " LLC queue length: %zu%s", ms->llc_queue()->size(), VTY_NEWLINE); - vty_out(vty, " LLC queue octets: %d%s", ms->llc_queue()->octets(), + vty_out(vty, " LLC queue octets: %zu%s", ms->llc_queue()->octets(), VTY_NEWLINE); if (ms->l1_meas()->have_rssi) vty_out(vty, " RSSI: %d dBm%s", -- 2.6.2

Holger Freyther

15 Jan 15 Jan

3:39 p.m.

New subject: [PATCH 2/2] fix some format specifiers

...

On 06 Nov 2015, at 21:00, Alexander Huemer <alexander.huemer(a)xx.vu> wrote:

Dear Jacob, this seems to still apply. Could you please review and update the case on patchwork? kind regards holger

...

Holger Freyther

6 Nov 6 Nov

9:09 p.m.

...

On 06 Nov 2015, at 20:48, Alexander Huemer <alexander.huemer(a)xx.vu> wrote: Hi, here are some patches that I produced while compiling some osmocom projects and its dependencies on a new box. Most of them are trivialities that prevent compiler warnings. The patch for openbsc though fixes a bug that prevents the build when libgtp from openggsn is not present on the system.

ah cool! I will try to have a look this weekend. holger

Alexander Huemer

9:24 p.m.

Hi, while fixing some trivial compiler warnings in openbsc I stumbled over this:

...

Making all in abis CC abis_test.o abis_test.c: In function ‘test_simple_sw_config’: abis_test.c:68:9: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 2 has type ‘long int’ [-Wformat=] printf("Start: %u len: %zu\n", descr[0].start - simple_config, descr[0].len); ^ abis_test.c: In function ‘test_dual_sw_config’: abis_test.c:111:9: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 2 has type ‘long int’ [-Wformat=] printf("Start: %u len: %zu\n", descr[0].start - dual_config, descr[0].len); ^ abis_test.c:115:9: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 2 has type ‘long int’ [-Wformat=] printf("Start: %u len: %zu\n", descr[1].start - dual_config, descr[1].len); ^ abis_test.c: In function ‘test_sw_selection’: abis_test.c:132:9: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 2 has type ‘long int’ [-Wformat=] printf("Start: %u len: %zu\n", descr[0].start - load_config, descr[0].len); ^ abis_test.c:136:9: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 2 has type ‘long int’ [-Wformat=] printf("Start: %u len: %zu\n", descr[1].start - load_config, descr[1].len);

The relevant expression is: descr[1].start - load_config and variations thereof. descr[1].start is declared include/openbsc/abis_nm.h as: const uint8_t *start; simple_config is declared as: static const uint8_t simple_config[] So an address is taken and the first element (implicitly) of an uint8 array is substracted from it. Maybe I just don't get the beauty of this, for me this looks wrong. Kind regards, -Alex

Neels Hofmeyr

7 Nov 7 Nov

3:42 a.m.

On Fri, Nov 06, 2015 at 09:24:40PM +0100, Alexander Huemer wrote:

...

Hi, while fixing some trivial compiler warnings in openbsc I stumbled over this:

No, not at all :) simple_config, a uint8_t*, is passed to abis_nm_parse_sw_config. After that, uint8_t *start is actually pointing at a byte within simple_config. It's a subtraction of two uint8_t* from each other to get a byte offset: Subtract simple_config's address (the start of the data) from the "start" pointer's address value, and the result is a byte offset to get from simple_config to where "start" is pointing. The declaration static const uint8_t simple_config[] yields a uint8_t*, but declaring it with [] allows initializing a static array. (Also, simple_config == &simple_config == &simple_config[0], all uint8_t*, which spaces me out every time I come across it.) Also note the slight counter intuitiveness of subtracting pointers to get byte offsets: (T*)a - (T*)b == ((uint)a - (uint)b))/sizeof(T), so this case gives byte offsets precisely because sizeof(uint8_t) == 1... ~Neels

Alexander Huemer

1:39 p.m.

Hi! On Sat, Nov 07, 2015 at 03:42:55AM +0100, Neels Hofmeyr wrote:

...

On Fri, Nov 06, 2015 at 09:24:40PM +0100, Alexander Huemer wrote:

Yes, indeed, thanks for the clarification. So what happens here is pointer arithmetic. I misinterpreted the right side of the '-'. Sorry for the noise. Kind regards, -Alex

Alexander Huemer

2 p.m.

New subject: [PATCH 0/1] A patch for openbsc

Hi, so I guess the right format specifier to use here is "%td", since the expression is (something like) a ptrdiff_t. At least this fixes the warning and `make check` still passes the abis test. Kind regards, -Alex

Alexander Huemer

2 p.m.

New subject: [PATCH] tests/abis: fix format specifiers

--- openbsc/tests/abis/abis_test.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/openbsc/tests/abis/abis_test.c b/openbsc/tests/abis/abis_test.c index 53e8a4c..496267f 100644 --- a/openbsc/tests/abis/abis_test.c +++ b/openbsc/tests/abis/abis_test.c @@ -65,7 +65,7 @@ static void test_simple_sw_config(void) abort(); } - printf("Start: %u len: %zu\n", descr[0].start - simple_config, descr[0].len); + printf("Start: %td len: %zu\n", descr[0].start - simple_config, descr[0].len); printf("file_id: %s\n", osmo_hexdump(descr[0].file_id, descr[0].file_id_len)); printf("file_ver: %s\n", osmo_hexdump(descr[0].file_ver, descr[0].file_ver_len)); } @@ -108,11 +108,11 @@ static void test_dual_sw_config(void) abort(); } - printf("Start: %u len: %zu\n", descr[0].start - dual_config, descr[0].len); + printf("Start: %td len: %zu\n", descr[0].start - dual_config, descr[0].len); printf("file_id: %s\n", osmo_hexdump(descr[0].file_id, descr[0].file_id_len)); printf("file_ver: %s\n", osmo_hexdump(descr[0].file_ver, descr[0].file_ver_len)); - printf("Start: %u len: %zu\n", descr[1].start - dual_config, descr[1].len); + printf("Start: %td len: %zu\n", descr[1].start - dual_config, descr[1].len); printf("file_id: %s\n", osmo_hexdump(descr[1].file_id, descr[1].file_id_len)); printf("file_ver: %s\n", osmo_hexdump(descr[1].file_ver, descr[1].file_ver_len)); } @@ -129,11 +129,11 @@ static void test_sw_selection(void) abort(); } - printf("Start: %u len: %zu\n", descr[0].start - load_config, descr[0].len); + printf("Start: %td len: %zu\n", descr[0].start - load_config, descr[0].len); printf("file_id: %s\n", osmo_hexdump(descr[0].file_id, descr[0].file_id_len)); printf("file_ver: %s\n", osmo_hexdump(descr[0].file_ver, descr[0].file_ver_len)); - printf("Start: %u len: %zu\n", descr[1].start - load_config, descr[1].len); + printf("Start: %td len: %zu\n", descr[1].start - load_config, descr[1].len); printf("file_id: %s\n", osmo_hexdump(descr[1].file_id, descr[1].file_id_len)); printf("file_ver: %s\n", osmo_hexdump(descr[1].file_ver, descr[1].file_ver_len)); -- 2.6.2

Neels Hofmeyr

18 Nov 18 Nov

3:29 p.m.

New subject: [PATCH 0/1] A patch for openbsc

On Sat, Nov 07, 2015 at 02:00:51PM +0100, Alexander Huemer wrote:

...

so I guess the right format specifier to use here is "%td", since the expression is (something like) a ptrdiff_t.

Thanks, it's actually the first time I come across ptrdiff_t. From the explanations I've found, this is The Correct (TM) type to use, indeed. In my /usr/lib/gcc/x86_64-linux-gnu it's defined as long int. I ask myself though -- for linkage size smaller than 2 GB, apparently the default everywhere, pointer differences will stay within the 32 bit address range, and using an int should suffice? To link surpassing 2 GB, one needs to explicitly pass -mcmodel=medium to gcc, and I doubt any osmo project will surpass 2 GB linkage any time soon ;) Instead of reading man snprintf all the time, I actually tend to use %d and cast the argument to (int) :P This will only work if the thing to print is entirely within the int range, of course. uint16_t x = 5; printf("%d", (int)x); An advantage here is that the type of x can be tweaked later without having to edit the formats everywhere (error prone). I can't really find them now, but I dimly remember seeing some osmo code here and there where printf() formats could use some more attention... Like, is it accurate to pass an enum type to %d without casting? And so on. Most of those instances still print the correct value because, I assume, actual storage of the values is often blown up to the CPU's native size... (handwavy) ~Neels

Alexander Huemer

6 Nov 6 Nov

9:35 p.m.

Hi, gcc-5.2.1 from Debian stretch refuses to build osmo-pcu.

...

bts.cpp:86:1: error: invalid conversion from 'const rate_ctr_desc*' to 'unsigned int' [-fpermissive] };

The relevant piece of code from bts.cpp is: static const struct rate_ctr_group_desc bts_ctrg_desc = { "bts", "BTS Statistics", ARRAY_SIZE(bts_ctr_description), bts_ctr_description, }; This structure comes from libosmocore, osmocom/core/rate_ctr.h, without comments: struct rate_ctr_group_desc { const char *group_name_prefix; const char *group_description; int class_id; const unsigned int num_ctr; const struct rate_ctr_desc *ctr_desc; }; It seems like the class_id is missing. Since the initialization is done in the classic, ordered way, the last three fields are initialized to unintened values, at least that's what I make of this. For me that looks like a real bug. Kind regards, -Alex

Holger Freyther

9:57 p.m.

...

On 06 Nov 2015, at 21:35, Alexander Huemer <alexander.huemer(a)xx.vu> wrote: Hi, For me that looks like a real bug.

real bug! and fails to compiler with earlier GCC as well (as on our jenkins). I wonder if there is momentum for Gerrit+Jenkins or Gitlab/Github+Travis to catch these things before we merge/integrate changes. :)

3028

days inactive

3098

days old

openbsc@lists.osmocom.org

Manage subscription

35 comments

3 participants

tags (0)

participants (3)

Alexander Huemer
Holger Freyther
Neels Hofmeyr