[PATCH 1/2] LOG: use uint8_t for pointer manipulation

[Angus Salkeld]

char changes size on different arches.

Signed-off-by: Angus Salkeld <asalkeld(a)redhat.com&gt;
---
 lib/log_blackbox.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/log_blackbox.c b/lib/log_blackbox.c
index 1e21f74..60f9e52 100644
--- a/lib/log_blackbox.c
+++ b/lib/log_blackbox.c
@@ -135,7 +135,7 @@ void qb_log_blackbox_print_from_file(const char *bb_filename)
 {
 	qb_ringbuffer_t *instance;
 	ssize_t bytes_read;
-	char chunk[512];
+	uint8_t chunk[512];
 	int fd;
 	char time_buf[64];
 
@@ -151,10 +151,10 @@ void qb_log_blackbox_print_from_file(const char *bb_filename)
 	}
 
 	do {
-		char *ptr;
+		uint8_t *ptr;
 		uint32_t *lineno;
 		uint32_t *fn_size;
-		char *function;
+		uint8_t *function;
 		time_t *timestamp;
 		/*uint32_t *log_size;*/
 
-- 
1.7.5.2

[PATCH 2/2] LOOP: make the return more consistent in qb_loop_timer_expire_time_get()

[Angus Salkeld]

If there is any error return 0
 (meaning the timer is or has expired)

Signed-off-by: Angus Salkeld <asalkeld(a)redhat.com&gt;
---
 include/qb/qbloop.h  |    2 ++
 lib/loop_poll.c      |    2 +-
 lib/loop_timerlist.c |    4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/qb/qbloop.h b/include/qb/qbloop.h
index 3b74616..6598885 100644
--- a/include/qb/qbloop.h
+++ b/include/qb/qbloop.h
@@ -136,6 +136,8 @@ int32_t qb_loop_timer_del(qb_loop_t *l, qb_loop_timer_handle th);
 /**
  * Get the time remaining before it expires.
  *
+ * @note if the timer has already expired it will return 0
+ *
  * @param l pointer to the loop instance
  * @param th timer handle.
  * @return nano seconds left
diff --git a/lib/loop_poll.c b/lib/loop_poll.c
index 3eabd99..e60b51d 100644
--- a/lib/loop_poll.c
+++ b/lib/loop_poll.c
@@ -805,7 +805,7 @@ uint64_t qb_loop_timer_expire_time_get(struct qb_loop *l,
qb_loop_timer_handle t
 	s = (struct qb_poll_source *)l->fd_source;
 	res = _poll_entry_from_handle_(s, th, &pe);
 	if (res != 0) {
-		return res;
+		return 0;
 	}
 
 	if (timerfd_gettime(pe->ufd.fd, &its) == -1) {
diff --git a/lib/loop_timerlist.c b/lib/loop_timerlist.c
index 2579714..2792ec9 100644
--- a/lib/loop_timerlist.c
+++ b/lib/loop_timerlist.c
@@ -246,11 +246,11 @@ uint64_t qb_loop_timer_expire_time_get(struct qb_loop *l,
qb_loop_timer_handle t
 
 	res = _timer_from_handle_(s, th, &t);
 	if (res != 0) {
-		return res;
+		return 0;
 	}
 
 	if (t->state != QB_POLL_ENTRY_ACTIVE) {
-		return -EBADF;
+		return 0;
 	}
 
 	return timerlist_expire_time (&s->timerlist, t->timerlist_handle);
-- 
1.7.5.2

[Jim Meyering]

Angus Salkeld wrote:
>  char changes size on different arches. 
Hi Angus,

Have you encountered an actual system on which "sizeof char" is not 1?

>  diff --git a/lib/log_blackbox.c b/lib/log_blackbox.c
>  index 1e21f74..60f9e52 100644
>  --- a/lib/log_blackbox.c
>  +++ b/lib/log_blackbox.c
>  @@ -135,7 +135,7 @@ void qb_log_blackbox_print_from_file(const char *bb_filename)
>   {
>   	qb_ringbuffer_t *instance;
>   	ssize_t bytes_read;
>  -	char chunk[512];
>  +	uint8_t chunk[512];
>   	int fd;
>   	char time_buf[64];
>
>  @@ -151,10 +151,10 @@ void qb_log_blackbox_print_from_file(const char *bb_filename)
>   	}
>
>   	do {
>  -		char *ptr;
>  +		uint8_t *ptr;
>   		uint32_t *lineno;
>   		uint32_t *fn_size;
>  -		char *function;
>  +		uint8_t *function;
>   		time_t *timestamp;
>   		/*uint32_t *log_size;*/

[Angus Salkeld]

On Thu, Jun 23, 2011 at 11:59:17AM +0200, Jim Meyering wrote:
>  Angus Salkeld wrote:
>  > char changes size on different arches.
>  
>  Hi Angus,
>  
>  Have you encountered an actual system on which "sizeof char" is not 1? 
No, I get that sizeof(char) is 1 on pretty much all systems.
(I was more concerned with sizeof(char*) vs. sizeof(uint32_t*)

I was trying to solve a sparc 64 compile warning. But since re-checking
it seems I have failed :(

On sparc64 sizeof(char*) == 8 and sizeof(int*) is 4 (I expected uint8_t
to be 4 but it much be 8).

The warning is:
log_blackbox.c: In function 'qb_log_blackbox_print_from_file':
log_blackbox.c:171:13: warning: cast increases required alignment of target type
[-Wcast-align]
log_blackbox.c:175:14: warning: cast increases required alignment of target type
[-Wcast-align]
log_blackbox.c:182:16: warning: cast increases required alignment of target type
[-Wcast-align]

sparc64:
http://sparc.koji.fedoraproject.org/koji/getfile?taskID=112209&name=b...
sparcv9:
http://sparc.koji.fedoraproject.org/koji/getfile?taskID=112210&name=b...
The test fails on sparcv9 - but I am not sure how to get at the logs.

Any way I am very interested in the "correcct" way of fixing that
warning.

-Angus

>  
>  > diff --git a/lib/log_blackbox.c b/lib/log_blackbox.c
>  > index 1e21f74..60f9e52 100644
>  > --- a/lib/log_blackbox.c
>  > +++ b/lib/log_blackbox.c
>  > @@ -135,7 +135,7 @@ void qb_log_blackbox_print_from_file(const char *bb_filename)
>  >  {
>  >  	qb_ringbuffer_t *instance;
>  >  	ssize_t bytes_read;
>  > -	char chunk[512];
>  > +	uint8_t chunk[512];
>  >  	int fd;
>  >  	char time_buf[64];
>  >
>  > @@ -151,10 +151,10 @@ void qb_log_blackbox_print_from_file(const char *bb_filename)
>  >  	}
>  >
>  >  	do {
>  > -		char *ptr;
>  > +		uint8_t *ptr;
>  >  		uint32_t *lineno;
>  >  		uint32_t *fn_size;
>  > -		char *function;
>  > +		uint8_t *function;
>  >  		time_t *timestamp;
>  >  		/*uint32_t *log_size;*/
>  _______________________________________________
>  quarterback-devel mailing list
>  quarterback-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/quarterback-devel

[Jim Meyering]

Angus Salkeld wrote:

>  On Thu, Jun 23, 2011 at 11:59:17AM +0200, Jim Meyering wrote:
> > Angus Salkeld wrote:
> > > char changes size on different arches.
> >
> > Hi Angus,
> >
> > Have you encountered an actual system on which "sizeof char" is not 1?
>
>  No, I get that sizeof(char) is 1 on pretty much all systems.
>  (I was more concerned with sizeof(char*) vs. sizeof(uint32_t*)
>
>  I was trying to solve a sparc 64 compile warning. But since re-checking
>  it seems I have failed :(
>
>  On sparc64 sizeof(char*) == 8 and sizeof(int*) is 4 (I expected uint8_t
>  to be 4 but it much be 8).
>
>  The warning is:
>  log_blackbox.c: In function 'qb_log_blackbox_print_from_file':
>  log_blackbox.c:171:13: warning: cast increases required alignment of target type
> [-Wcast-align]
>  log_blackbox.c:175:14: warning: cast increases required alignment of target type
> [-Wcast-align]
>  log_blackbox.c:182:16: warning: cast increases required alignment of target type
> [-Wcast-align]
>
>  sparc64:
> http://sparc.koji.fedoraproject.org/koji/getfile?taskID=112209&name=b...
>  sparcv9:
> http://sparc.koji.fedoraproject.org/koji/getfile?taskID=112210&name=b...
>  The test fails on sparcv9 - but I am not sure how to get at the logs.
>
>  Any way I am very interested in the "correcct" way of fixing that
>  warning. 
Hi Angus,

I took a look but was quickly distracted by something else.
An invalid timestamp could cause a NULL dereference.
Also, if this logging code may be run in a multi-threading
context, then it should be changed to use localtime_r rather
than the thread-unsafe localtime function.

> From 7ce371b61a14e784a283ae4523b29d63fe3995bf Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com&gt;
Date: Fri, 24 Jun 2011 19:19:10 +0200
Subject: [PATCH] don't let an invalid time stamp provoke a NULL dereference

* lib/log_blackbox.c (qb_log_blackbox_print_from_file): Handle
localtime failure by printing the long-int representation rather
than a strftime-formatted one.
---
 lib/log_blackbox.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/log_blackbox.c b/lib/log_blackbox.c
index be26b79..1696614 100644
--- a/lib/log_blackbox.c
+++ b/lib/log_blackbox.c
@@ -167,6 +167,7 @@ qb_log_blackbox_print_from_file(const char *bb_filename)
 		bytes_read = qb_rb_chunk_read(instance, chunk, 512, 0);
 		ptr = chunk;
 		if (bytes_read > 0) {
+			struct tm *tm;
 			/* lineno */
 			lineno = (uint32_t *) ptr;
 			ptr += sizeof(uint32_t);
@@ -181,8 +182,13 @@ qb_log_blackbox_print_from_file(const char *bb_filename)
 			/* timestamp size & content */
 			timestamp = (time_t *) ptr;
 			ptr += sizeof(time_t);
-			(void)strftime(time_buf, sizeof(time_buf), "%b %d %T",
-				       localtime(timestamp));
+			tm = localtime(timestamp);
+			if (tm)
+				strftime(time_buf, sizeof(time_buf), "%b %d %T",
+					 tm);
+			else
+				snprintf(time_buf, sizeof(time_buf), "%ld",
+					 (long int) timestamp);

 			/* message size & content */
 			/* log_size = (uint32_t *) ptr; */
--
1.7.6.rc2.302.gc2115

[Angus Salkeld]

On Fri, Jun 24, 2011 at 07:23:56PM +0200, Jim Meyering wrote:
>  
>  Hi Angus,
>  
>  I took a look but was quickly distracted by something else.
>  An invalid timestamp could cause a NULL dereference.
>  Also, if this logging code may be run in a multi-threading
>  context, then it should be changed to use localtime_r rather
>  than the thread-unsafe localtime function.
>   
Thanks, I tweeked it slightly to pass "make check". And applied.

-Angus

[Jim Meyering]

Angus Salkeld wrote:

>  On Thu, Jun 23, 2011 at 11:59:17AM +0200, Jim Meyering wrote:
> > Angus Salkeld wrote:
> > > char changes size on different arches.
> >
> > Hi Angus,
> >
> > Have you encountered an actual system on which "sizeof char" is not 1?
>
>  No, I get that sizeof(char) is 1 on pretty much all systems.
>  (I was more concerned with sizeof(char*) vs. sizeof(uint32_t*)
>
>  I was trying to solve a sparc 64 compile warning. But since re-checking
>  it seems I have failed :(
>
>  On sparc64 sizeof(char*) == 8 and sizeof(int*) is 4 (I expected uint8_t
>  to be 4 but it much be 8).
>
>  The warning is:
>  log_blackbox.c: In function 'qb_log_blackbox_print_from_file':
>  log_blackbox.c:171:13: warning: cast increases required alignment of target type
> [-Wcast-align]
>  log_blackbox.c:175:14: warning: cast increases required alignment of target type
> [-Wcast-align]
>  log_blackbox.c:182:16: warning: cast increases required alignment of target type
> [-Wcast-align] 
That's warning about a very real problem that is particularly
important on sparc.  On that architecture, you may not blindly
point an int* pointer, P, into a char* buffer and then dereference P.
If P is not properly aligned, dereferencing it will cause a fault.
Thus, to do what you want, you would have to have, say, an "int" variable
(say "I") and memcpy the desired bytes from the char* buffer into
"I".

Then you may safely use "I" in place of code where you now use *P.

The solution requires no casts.