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
src/stat_item.c | 2 +- utils/osmo-sim-test.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)
--- 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>
--- 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; }
On 06 Nov 2015, at 20:55, Alexander Huemer alexander.huemer@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?
On 07 Nov 2015, at 12:30, Holger Freyther holger@freyther.de wrote:
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?
@Jacob: ping?
include/osmocom/abis/lapd.h | 2 ++ 1 file changed, 2 insertions(+)
--- 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,
examples/lapd-over-datagram-network.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
--- 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); }
openbsc/tests/oap/Makefile.am | 4 ++++ 1 file changed, 4 insertions(+)
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
Makefile.am | 1 + configure.in => configure.ac | 0 gtp/gtp.c | 11 ++++++----- gtp/pdp.c | 7 ++++--- 4 files changed, 11 insertions(+), 8 deletions(-)
--- 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
--- 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
--- 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;
src/common/bts_ctrl_lookup.c | 1 + src/common/l1sap.c | 1 + src/common/msg_utils.c | 6 +++--- src/common/rsl.c | 1 + src/common/vty.c | 5 +++-- tests/agch/agch_test.c | 9 +++++---- 6 files changed, 14 insertions(+), 9 deletions(-)
--- 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,
--- 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
.gitignore | 2 ++ Makefile.am | 1 + configure.ac | 2 ++ m4/DUMMY | 0 src/pcu_vty_functions.cpp | 7 ++++--- 5 files changed, 9 insertions(+), 3 deletions(-)
--- .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
On 06 Nov 2015, at 21:00, Alexander Huemer alexander.huemer@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
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@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?
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
On 07 Nov 2015, at 14:07, Alexander Huemer alexander.huemer@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@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?
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.
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?
On Fri, Jan 15, 2016 at 03:37:27PM +0100, Holger Freyther wrote:
On 07 Nov 2015, at 14:07, Alexander Huemer alexander.huemer@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@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?
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.
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
--- 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",
On 06 Nov 2015, at 21:00, Alexander Huemer alexander.huemer@xx.vu wrote:
Dear Jacob,
this seems to still apply. Could you please review and update the case on patchwork?
kind regards holger
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
On 06 Nov 2015, at 20:48, Alexander Huemer alexander.huemer@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
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
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:
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_configand 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.
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
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:
The relevant expression is:
descr[1].start - load_configand 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.
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...
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
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
--- 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));
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
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
On 06 Nov 2015, at 21:35, Alexander Huemer alexander.huemer@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. :)
-
Alexander Huemer -
Holger Freyther -
Neels Hofmeyr