diff mbox

Fix bunch of printf warnings

Message ID 1413989408-14993-1-git-send-email-max.suraev@fairwaves.co
State Rejected
Headers show

Commit Message

Max Oct. 22, 2014, 2:50 p.m. UTC
Signed-off-by: Max <max.suraev@fairwaves.co>
---
 openbsc/src/libbsc/bsc_api.c         |  4 ++--
 openbsc/src/libmgcp/mgcp_protocol.c  |  2 +-
 openbsc/tests/abis/abis_test.c       | 16 ++++++++--------
 openbsc/tests/gbproxy/gbproxy_test.c | 16 ++++++++--------
 4 files changed, 19 insertions(+), 19 deletions(-)

Comments

Max Oct. 22, 2014, 3:15 p.m. UTC | #1
One step closer to build with -Werror :)
Holger Freyther Oct. 22, 2014, 9:32 p.m. UTC | #2
On Wed, Oct 22, 2014 at 04:50:08PM +0200, Max wrote:

Hi,

>  	gh = msgb_l3(msg);
>  	if (msgb_l3len(msg) - sizeof(*gh) != 1) {
> -		LOGP(DMSC, LOGL_ERROR, "Assignment Compl invalid: %u\n",
> +		LOGP(DMSC, LOGL_ERROR, "Assignment Compl invalid: %lu\n",
>  		     msgb_l3len(msg) - sizeof(*gh));

it looks like a candidate for ping-pong games on 32bit/64bit userland
on GNU/Linux. What is the type here? unsigned int - size_t?

holger
Max Oct. 23, 2014, 8:24 a.m. UTC | #3
Good catch, I didn't notice it. Maybe we should just use size_t for all the length
functions in libosmo*? That's what size_t was invented for after all :)

22.10.2014 23:32, Holger Hans Peter Freyther пишет:
> On Wed, Oct 22, 2014 at 04:50:08PM +0200, Max wrote:
> 
> Hi,
> 
>>  	gh = msgb_l3(msg);
>>  	if (msgb_l3len(msg) - sizeof(*gh) != 1) {
>> -		LOGP(DMSC, LOGL_ERROR, "Assignment Compl invalid: %u\n",
>> +		LOGP(DMSC, LOGL_ERROR, "Assignment Compl invalid: %lu\n",
>>  		     msgb_l3len(msg) - sizeof(*gh));
> 
> it looks like a candidate for ping-pong games on 32bit/64bit userland
> on GNU/Linux. What is the type here? unsigned int - size_t?
> 
> holger
>
Holger Freyther Oct. 23, 2014, 12:39 p.m. UTC | #4
On Thu, Oct 23, 2014 at 10:24:57AM +0200, ☎ wrote:

> Good catch, I didn't notice it. Maybe we should just use size_t for all the length
> functions in libosmo*? That's what size_t was invented for after all :)

> > it looks like a candidate for ping-pong games on 32bit/64bit userland
> > on GNU/Linux. What is the type here? unsigned int - size_t?

I can't quote the ISO C specification. So what type is the result of
"unsigned int - size_t"? We have size_t, off_t, ptrdiff_t as potential
types. I don't know which is the right one. :)
Jan Engelhardt Oct. 23, 2014, 12:47 p.m. UTC | #5
On Thursday 2014-10-23 14:39, Holger Hans Peter Freyther wrote:

>On Thu, Oct 23, 2014 at 10:24:57AM +0200, ☎ wrote:
>
>> Good catch, I didn't notice it. Maybe we should just use size_t for all the length
>> functions in libosmo*? That's what size_t was invented for after all :)
>
>> > it looks like a candidate for ping-pong games on 32bit/64bit userland
>> > on GNU/Linux. What is the type here? unsigned int - size_t?
>
>I can't quote the ISO C specification. So what type is the result of
>"unsigned int - size_t"?

size_t, because it is of higher rank.
(The specifier to use is %zu.)
Peter Stuge Oct. 23, 2014, 3:59 p.m. UTC | #6
Jan Engelhardt wrote:
> > So what type is the result of "unsigned int - size_t"?
> 
> size_t, because it is of higher rank.

How come it has higher rank?

I don't have the spec, maybe it is stated explicitly there, but this
blog post (it's on the internet so it MUST be true!) outlines rules
based on the standard, and doesn't mention either size_t type.

I have a feeling that size_t is not strictly defined, ie. that a
programmer or even a compiler is free to choose any representation,
as long as all memory addresses can be stored. In that case size_t
rank can't really be determined.

We could always cast... :)


//Peter
Max Oct. 23, 2014, 4:09 p.m. UTC | #7
23.10.2014 17:59, Peter Stuge пишет:
> 
> We could always cast... :)
> 

That's an evil witchcraft! :)
Holger Freyther Oct. 25, 2014, 1:42 p.m. UTC | #8
On Thu, Oct 23, 2014 at 05:59:32PM +0200, Peter Stuge wrote:

> How come it has higher rank?
> 
> I don't have the spec, maybe it is stated explicitly there, but this
> blog post (it's on the internet so it MUST be true!) outlines rules
> based on the standard, and doesn't mention either size_t type.

#include <stdint.h>
#include <sys/socket.h>
#include <stdint.h>

int foo()
{
        unsigned int foo = 1212323123123;
        size_t bla = 2;

        typeof(foo - bla) k;
        k = 3;
}


clang-3.4 -Xclang -ast-dump -fsyntax-only foo.c

`-FunctionDecl 0x9946300 </tmp/foo.c:5:1, line:12:1> foo 'int ()'
  `-CompoundStmt 0x9946588 <line:6:1, line:12:1>
    |-DeclStmt 0x99463c8 <line:7:2, col:34>
    | `-VarDecl 0x9946370 <col:2, col:21> foo 'unsigned int'
    |   `-ImplicitCastExpr 0x99463b8 <col:21> 'unsigned int' <IntegralCast>
    |     `-IntegerLiteral 0x99463a0 <col:21> 'long long' 1212323123123
    |-DeclStmt 0x9946438 <line:8:2, col:16>
    | `-VarDecl 0x99463e0 <col:2, col:15> bla 'size_t':'unsigned int'
    |   `-ImplicitCastExpr 0x9946428 <col:15> 'size_t':'unsigned int' <IntegralCast>
    |     `-IntegerLiteral 0x9946410 <col:15> 'int' 2
    |-DeclStmt 0x9946520 <line:10:2, col:21>
    | `-VarDecl 0x99464f0 <col:2, col:20> k 'typeof (foo - bla)':'unsigned int'
    `-BinaryOperator 0x9946570 <line:11:2, col:6> 'typeof (foo - bla)':'unsigned int' '='
      |-DeclRefExpr 0x9946530 <col:2> 'typeof (foo - bla)':'unsigned int' lvalue Var 0x99464f0 'k' 'typeof (foo - bla)':'unsigned int'
      `-ImplicitCastExpr 0x9946560 <col:6> 'typeof (foo - bla)':'unsigned int' <IntegralCast>
        `-IntegerLiteral 0x9946548 <col:6> 'int' 3

If I read this correctly the type is 'unsigned int' with clang? An
issue with my reading? Clang bug?


cheers
	holger
Jan Engelhardt Oct. 25, 2014, 3:41 p.m. UTC | #9
On Saturday 2014-10-25 15:42, Holger Hans Peter Freyther wrote:
>int foo()
>{
>        unsigned int foo = 1212323123123;
>        size_t bla = 2;
>
>        typeof(foo - bla) k;
>        k = 3;
>}
>
>clang-3.4 -Xclang -ast-dump -fsyntax-only foo.c
>    | `-VarDecl 0x99464f0 <col:2, col:20> k 'typeof (foo - bla)':'unsigned int'
>
>If I read this correctly the type is 'unsigned int' with clang?

You seem to be compiling in LP32 mode.
I get unsigned long -- which is what size_t amounts to on Linux/LP64.
Peter Stuge Oct. 26, 2014, 12:04 p.m. UTC | #10
Peter Stuge wrote:
> Jan Engelhardt wrote:
> > > So what type is the result of "unsigned int - size_t"?
> > 
> > size_t, because it is of higher rank.
> 
> How come it has higher rank?

I'd appreciate an answer to this. "because" isn't it.


//Peter
Jan Engelhardt Oct. 26, 2014, 5:29 p.m. UTC | #11
On Sunday 2014-10-26 13:04, Peter Stuge wrote:
>> > > So what type is the result of "unsigned int - size_t"?
>> > size_t, because it is of higher rank.
>> How come it has higher rank?
>I'd appreciate an answer to this. "because" isn't it.

Then you will be disappointed, because "because it is" is the answer - 
the standards body defined it to be that way.
https://www.securecoding.cert.org/confluence/display/seccode/INT02-C.+Understand+integer+conversion+rules
Peter Stuge Oct. 27, 2014, 7:41 a.m. UTC | #12
Jan Engelhardt wrote:
> >> > > So what type is the result of "unsigned int - size_t"?
> >> > size_t, because it is of higher rank.
> >> How come it has higher rank?
> >I'd appreciate an answer to this. "because" isn't it.
> 
> "because it is" is the answer -
> the standards body defined it to be that way.

It's not clear to me that this is the case.

> https://www.securecoding.cert.org/confluence/display/seccode/INT02-C.+Understand+integer+conversion+rules

Right, this is the blog post I referenced in my previous email. I
forgot to include the link. Thanks for sending it.

This post makes a reference to the standard and seems to repeat the
rank rules, but it doesn't seem to me that any of the rules apply to
the case at hand?


//Peter
diff mbox

Patch

diff --git a/openbsc/src/libbsc/bsc_api.c b/openbsc/src/libbsc/bsc_api.c
index 5ce0bc4..f4121de 100644
--- a/openbsc/src/libbsc/bsc_api.c
+++ b/openbsc/src/libbsc/bsc_api.c
@@ -416,7 +416,7 @@  static void handle_ass_compl(struct gsm_subscriber_connection *conn,
 
 	gh = msgb_l3(msg);
 	if (msgb_l3len(msg) - sizeof(*gh) != 1) {
-		LOGP(DMSC, LOGL_ERROR, "Assignment Compl invalid: %u\n",
+		LOGP(DMSC, LOGL_ERROR, "Assignment Compl invalid: %lu\n",
 		     msgb_l3len(msg) - sizeof(*gh));
 		return;
 	}
@@ -461,7 +461,7 @@  static void handle_ass_fail(struct gsm_subscriber_connection *conn,
 
 	gh = msgb_l3(msg);
 	if (msgb_l3len(msg) - sizeof(*gh) != 1) {
-		LOGP(DMSC, LOGL_ERROR, "assignemnt failure unhandled: %u\n",
+		LOGP(DMSC, LOGL_ERROR, "assignemnt failure unhandled: %lu\n",
 		     msgb_l3len(msg) - sizeof(*gh));
 		rr_failure = NULL;
 	} else {
diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c
index 79422fe..00315fb 100644
--- a/openbsc/src/libmgcp/mgcp_protocol.c
+++ b/openbsc/src/libmgcp/mgcp_protocol.c
@@ -308,7 +308,7 @@  static int write_response_sdp(struct mgcp_endpoint *endp,
 	return len;
 
 buffer_too_small:
-	LOGP(DMGCP, LOGL_ERROR, "SDP buffer too small: %d (needed %d)\n",
+	LOGP(DMGCP, LOGL_ERROR, "SDP buffer too small: %zu (needed %d)\n",
 	     size, len);
 	return -1;
 }
diff --git a/openbsc/tests/abis/abis_test.c b/openbsc/tests/abis/abis_test.c
index e7e78d2..f82740f 100644
--- a/openbsc/tests/abis/abis_test.c
+++ b/openbsc/tests/abis/abis_test.c
@@ -61,11 +61,11 @@  static void test_simple_sw_config(void)
 	}
 
 	if (descr[0].len != 13) {
-		printf("WRONG SIZE: %d\n", descr[0].len);
+		printf("WRONG SIZE: %zu\n", descr[0].len);
 		abort();
 	}
 
-	printf("Start: %u len: %zu\n", descr[0].start - simple_config, descr[0].len);
+	printf("Start: %lu 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));
 }
@@ -99,20 +99,20 @@  static void test_dual_sw_config(void)
 	}
 
 	if (descr[0].len != 13) {
-		printf("WRONG SIZE0: %d\n", descr[0].len);
+		printf("WRONG SIZE0: %zu\n", descr[0].len);
 		abort();
 	}
 
 	if (descr[1].len != 13) {
-		printf("WRONG SIZE1: %d\n", descr[1].len);
+		printf("WRONG SIZE1: %zu\n", descr[1].len);
 		abort();
 	}
 
-	printf("Start: %u len: %zu\n", descr[0].start - dual_config, descr[0].len);
+	printf("Start: %lu 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: %lu 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: %lu 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: %lu 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));
 
diff --git a/openbsc/tests/gbproxy/gbproxy_test.c b/openbsc/tests/gbproxy/gbproxy_test.c
index 5aa301b..161f7fd 100644
--- a/openbsc/tests/gbproxy/gbproxy_test.c
+++ b/openbsc/tests/gbproxy/gbproxy_test.c
@@ -898,7 +898,7 @@  int gprs_ns_rcvmsg(struct gprs_ns_inst *nsi, struct msgb *msg,
 int gprs_ns_callback(enum gprs_ns_evt event, struct gprs_nsvc *nsvc,
 			 struct msgb *msg, uint16_t bvci)
 {
-	printf("CALLBACK, event %d, msg length %d, bvci 0x%04x\n%s\n\n",
+	printf("CALLBACK, event %d, msg length %zu, bvci 0x%04x\n%s\n\n",
 			event, msgb_bssgp_len(msg), bvci,
 			osmo_hexdump(msgb_l2(msg), msgb_l2len(msg)));
 
@@ -925,15 +925,15 @@  ssize_t sendto(int sockfd, const void *buf, size_t len, int flags,
 		real_sendto = dlsym(RTLD_NEXT, "sendto");
 
 	if (dest_host == REMOTE_BSS_ADDR)
-		printf("MESSAGE to BSS at 0x%08x:%d, msg length %d\n%s\n\n",
+		printf("MESSAGE to BSS at 0x%08x:%d, msg length %zu\n%s\n\n",
 		       dest_host, dest_port,
 		       len, osmo_hexdump(buf, len));
 	else if (dest_host == REMOTE_SGSN_ADDR)
-		printf("MESSAGE to SGSN at 0x%08x:%d, msg length %d\n%s\n\n",
+		printf("MESSAGE to SGSN at 0x%08x:%d, msg length %zu\n%s\n\n",
 		       dest_host, dest_port,
 		       len, osmo_hexdump(buf, len));
 	else if (dest_host == REMOTE_SGSN2_ADDR)
-		printf("MESSAGE to SGSN 2 at 0x%08x:%d, msg length %d\n%s\n\n",
+		printf("MESSAGE to SGSN 2 at 0x%08x:%d, msg length %zu\n%s\n\n",
 		       dest_host, dest_port,
 		       len, osmo_hexdump(buf, len));
 	else
@@ -957,15 +957,15 @@  int gprs_ns_sendmsg(struct gprs_ns_inst *nsi, struct msgb *msg)
 
 	if (nsei == SGSN_NSEI)
 		printf("NS UNITDATA MESSAGE to SGSN, BVCI 0x%04x, "
-		       "msg length %d (%s)\n",
+		       "msg length %zu (%s)\n",
 		       bvci, len, __func__);
 	else if (nsei == SGSN2_NSEI)
 		printf("NS UNITDATA MESSAGE to SGSN 2, BVCI 0x%04x, "
-		       "msg length %d (%s)\n",
+		       "msg length %zu (%s)\n",
 		       bvci, len, __func__);
 	else
 		printf("NS UNITDATA MESSAGE to BSS, BVCI 0x%04x, "
-		       "msg length %d (%s)\n",
+		       "msg length %zu (%s)\n",
 		       bvci, len, __func__);
 
 	if (received_messages) {
@@ -1195,7 +1195,7 @@  static int gprs_process_message(struct gprs_ns_inst *nsi, const char *text, stru
 	struct msgb *msg;
 	int ret;
 	if (data_len > NS_ALLOC_SIZE - NS_ALLOC_HEADROOM) {
-		fprintf(stderr, "message too long: %d\n", data_len);
+		fprintf(stderr, "message too long: %zu\n", data_len);
 		return -1;
 	}