Skip to contentSkip to conversation

*** fix gcc warnings ***

PATCH In review Luca Berra 3 participants 8 messages Feb 27, 2010 – Mar 3, 2010 MD RAID

lore
Share & tools
lore

Luca Berra

author
#0 lore raw

cover *** fix gcc warnings ***

building current git with -O2 -Wall -Werror aborts with compiler warnings patch 1 fixes a strict-aliasing violation
patch 2 addresses trivial to fix warning
patch 3 is just _WRONG_: there is no clear way to bailout from the functions in Grow.c, and i have no idea if aborting would leave the array in an inconsistent state. For the time being I just decided to please gcc by storing the result somwere, but it is never checked.

L.

Luca Berra (3):
fix gcc warnings about strict-aliasing rules
fix compiler warnings
workaround unused_results

 Grow.c     |   30 +++++++++++++++++++-----------
 mdmon.c    |    2 +-
 restripe.c |    2 +-
 util.c     |    4 ++--
 4 files changed, 23 insertions(+), 15 deletions(-)
Signature · 6 lines
Luca Berra -- [email protected]
        Communication Media & Services S.r.l.
 /"\
 \ /     ASCII RIBBON CAMPAIGN
  X        AGAINST HTML MAIL
 / \
author
#1 lore raw

PATCH 1/3 fix gcc warnings about strict-aliasing rules

Signed-off-by Luca Berra <[email protected]>
---
  util.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)
util.c +2 -2
index 68f048d..1def2a0 100644
@@ -1160,7 +1160,7 @@ static int get_gpt_last_partition_end(int fd, unsigned long long *endofpart)
  	entry_size = __le32_to_cpu(buf[GPT_ENTRY_SIZE_OFFSET]);
  
  	/* Check GPT signature*/
-	if (*((__u64*)buf) != GPT_SIGNATURE_MAGIC)
+	if ((__u64)buf[0] != GPT_SIGNATURE_MAGIC)
  		return -1;
  
  	/* sanity checks */

@@ -1178,7 +1178,7 @@ static int get_gpt_last_partition_end(int fd, unsigned long long *endofpart)
  		/* is this valid partition? */
  		if (memcmp(part->type_guid, empty_gpt_entry, 16) != 0) {
  			/* check the last lba for the current partition */
-			curr_part_end = __le64_to_cpu(*(__u64*)part->ending_lba);
+			curr_part_end = __le64_to_cpu((__u64)part->ending_lba[0]);
  			if (curr_part_end > *endofpart)
  				*endofpart = curr_part_end;
  		}
Signature · 9 lines
1.7.0

-- 
Luca Berra -- [email protected]
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \
#2 lore raw

Re: [mdadm PATCH 1/3] fix gcc warnings about strict-aliasing rules

Luca Berra wrong:

Quoted 10 lines
Signed-off-by Luca Berra <[email protected]>
---
  util.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)
util.c +0 -0
index 68f048d..1def2a0 100644

util.c static int get_gpt_last_partition_end(int fd, · line 1163

Quoted 5 lines
util.c +1 -1
@@ -1160,7 +1160,7 @@ static int get_gpt_last_partition_end(int fd,
 unsigned long long *endofpart)
      entry_size = __le32_to_cpu(buf[GPT_ENTRY_SIZE_OFFSET]);
        /* Check GPT signature*/
-    if (*((__u64*)buf) != GPT_SIGNATURE_MAGIC)
+    if ((__u64)buf[0] != GPT_SIGNATURE_MAGIC)

This looks wrong.

buf is an array of unsigned char. Before, we converted the whole thing to a pointer to u64 and took the first element at that address, u64 size. Now after the change, we take first _byte_ of the array, convert it to u64 (adding leading zeros) and compare with a large number.

Does it actually work?

util.c static int get_gpt_last_partition_end(int fd, · line 1182

Quoted 9 lines
util.c +1 -1
          return -1;
        /* sanity checks */

@@ -1178,7 +1178,7 @@ static int get_gpt_last_partition_end(int fd,
 unsigned long long *endofpart)
          /* is this valid partition? */
          if (memcmp(part->type_guid, empty_gpt_entry, 16) != 0) {
              /* check the last lba for the current partition */
-            curr_part_end = __le64_to_cpu(*(__u64*)part->ending_lba);
+            curr_part_end = __le64_to_cpu((__u64)part->ending_lba[0]);

And the same thing here as well.

Thanks!

/mjt

Quoted 3 lines
util.c +0 -0
              if (curr_part_end > *endofpart)
                  *endofpart = curr_part_end;

          }
author
#3 lore raw

Re: [mdadm PATCH 1/3] fix gcc warnings about strict-aliasing rules

Quoted 19 lines · Michael Tokarev

Luca Berra wrong:

Quoted 10 lines
Signed-off-by Luca Berra <[email protected]>
---
  util.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)
util.c +0 -0
index 68f048d..1def2a0 100644

util.c static int get_gpt_last_partition_end(int fd, · line 1163

Quoted 5 lines
util.c +1 -1
@@ -1160,7 +1160,7 @@ static int get_gpt_last_partition_end(int fd,
 unsigned long long *endofpart)
      entry_size = __le32_to_cpu(buf[GPT_ENTRY_SIZE_OFFSET]);
        /* Check GPT signature*/
-    if (*((__u64*)buf) != GPT_SIGNATURE_MAGIC)
+    if ((__u64)buf[0] != GPT_SIGNATURE_MAGIC)

This looks wrong.

Quoted 5 lines

buf is an array of unsigned char. Before, we converted the whole thing to a pointer to u64 and took the first element at that address, u64 size. Now after the change, we take first _byte_ of the array, convert it to u64 (adding leading zeros) and compare with a large number.

you are correct, this is obviously wrong, i wonder what i was thinking.

any idea on how to get this right?
sscanf? union?

Regards,
L.

Signature · 6 lines
Luca Berra -- [email protected]
        Communication Media & Services S.r.l.
 /"\
 \ /     ASCII RIBBON CAMPAIGN
  X        AGAINST HTML MAIL
 / \
#4 lore raw

Re: [mdadm PATCH 1/3] fix gcc warnings about strict-aliasing rules

Quoted 26 lines · On Tue, 2 Mar 2010 08:48:11 +0100 Luca Berra <[email protected]> wrote:
Quoted 19 lines · Michael Tokarev

Luca Berra wrong:

Quoted 10 lines
Signed-off-by Luca Berra <[email protected]>
---
  util.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)
util.c +0 -0
index 68f048d..1def2a0 100644

util.c static int get_gpt_last_partition_end(int fd, · line 1163

Quoted 5 lines
util.c +1 -1
@@ -1160,7 +1160,7 @@ static int get_gpt_last_partition_end(int fd,
 unsigned long long *endofpart)
      entry_size = __le32_to_cpu(buf[GPT_ENTRY_SIZE_OFFSET]);
        /* Check GPT signature*/
-    if (*((__u64*)buf) != GPT_SIGNATURE_MAGIC)
+    if ((__u64)buf[0] != GPT_SIGNATURE_MAGIC)

This looks wrong.

Quoted 5 lines

buf is an array of unsigned char. Before, we converted the whole thing to a pointer to u64 and took the first element at that address, u64 size. Now after the change, we take first _byte_ of the array, convert it to u64 (adding leading zeros) and compare with a large number.

Quoted 4 lines

you are correct, this is obviously wrong, i wonder what i was thinking.

any idea on how to get this right?
sscanf? union?

I fixed it with:

__u64* buf64;

....
buf64 = (__u64*)buf;
if (buf64[0] != ....).....

NeilBrown

author
#5 lore raw

PATCH 2/3 fix compiler warnings

In reply to Luca Berra

one instance of uninitialized variable.
unused results from read/write which we don't care about. one instance of unused result from posix-memalign which is checked further down in the code.

Signed-off-by Luca Berra <[email protected]>
---
  Grow.c     |    7 ++++---
  mdmon.c    |    2 +-
  restripe.c |    2 +-
  3 files changed, 6 insertions(+), 5 deletions(-)
Grow.c +4 -3
index 74b63b6..5806fc3 100644
@@ -1096,7 +1096,7 @@ int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
  			/* set them all just in case some old 'new_*' value
  			 * persists from some earlier problem
  			 */
-			int err;
+			int err = 0;
  			if (sysfs_set_num(sra, NULL, "chunk_size", nchunk) < 0)
  				rv = 1, err = errno;
  			if (!rv && sysfs_set_num(sra, NULL, "layout", nlayout) < 0)

@@ -1414,8 +1414,9 @@ int wait_backup(struct mdinfo *sra,
  
  static void fail(char *msg)
  {
-	write(2, msg, strlen(msg));
-	write(2, "\n", 1);
+	ssize_t rc;
+	rc = write(2, msg, strlen(msg));
+	rc = write(2, "\n", 1);
  	exit(1);
  }
mdmon.c +1 -1
index 31d45fd..70fbd0b 100644
@@ -176,7 +176,7 @@ static void try_kill_monitor(pid_t pid, char *devname, int sock)
  	fl = fcntl(sock, F_GETFL, 0);
  	fl &= ~O_NONBLOCK;
  	fcntl(sock, F_SETFL, fl);
-	read(sock, buf, 100);
+	n = read(sock, buf, 100);
  }
  
  void remove_pidfile(char *devname)
restripe.c +1 -1
index f673206..d885e76 100644
@@ -565,7 +565,7 @@ int restore_stripes(int *dest, unsigned long long *offsets,
  
  	int data_disks = raid_disks - (level == 0 ? 0 : level <= 5 ? 1 : 2);
  
-	posix_memalign((void**)&stripe_buf, 4096, raid_disks * chunk_size);
+	i = posix_memalign((void**)&stripe_buf, 4096, raid_disks * chunk_size);
  	if (zero == NULL) {
  		zero = malloc(chunk_size);
  		if (zero)
Signature · 9 lines
1.7.0

-- 
Luca Berra -- [email protected]
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \
author
#6 lore raw

PATCH 3/3 workaround unused results

In reply to Luca Berra

errors from these tough unlikely should be checked when it is not clear how to bailout from a funcion just create a variable named discard_result and use it to please gcc

Signed-off-by Luca Berra <[email protected]>
---
  Grow.c |   23 +++++++++++++++--------
  1 files changed, 15 insertions(+), 8 deletions(-)
Grow.c +15 -8
index 5806fc3..8dbe139 100644
@@ -1269,6 +1269,7 @@ int grow_backup(struct mdinfo *sra,
  	int odata = disks;
  	int rv = 0;
  	int i;
+	ssize_t discard_result;
  	unsigned long long new_degraded;
  	//printf("offset %llu\n", offset);
  	if (level >= 4)

@@ -1334,10 +1335,10 @@ int grow_backup(struct mdinfo *sra,
  						((char*)&bsb.sb_csum2)-((char*)&bsb));
  
  		lseek64(destfd[i], destoffsets[i] - 4096, 0);
-		write(destfd[i], &bsb, 512);
+		discard_result = write(destfd[i], &bsb, 512);
  		if (destoffsets[i] > 4096) {
  			lseek64(destfd[i], destoffsets[i]+stripes*chunk*odata, 0);
-			write(destfd[i], &bsb, 512);
+			discard_result = write(destfd[i], &bsb, 512);
  		}
  		fsync(destfd[i]);
  	}

@@ -1368,6 +1369,7 @@ int wait_backup(struct mdinfo *sra,
  	int fd = sysfs_get_fd(sra, NULL, "sync_completed");
  	unsigned long long completed;
  	int i;
+	ssize_t discard_result;
  
  	if (fd < 0)
  		return -1;

@@ -1406,7 +1408,7 @@ int wait_backup(struct mdinfo *sra,
  			bsb.sb_csum2 = bsb_csum((char*)&bsb,
  						((char*)&bsb.sb_csum2)-((char*)&bsb));
  		lseek64(destfd[i], destoffsets[i]-4096, 0);
-		write(destfd[i], &bsb, 512);
+		discard_result = write(destfd[i], &bsb, 512);
  		fsync(destfd[i]);
  	}
  	return 0;

@@ -1453,8 +1455,10 @@ static void validate(int afd, int bfd, unsigned long long offset)
  			free(abuf);
  			free(bbuf);
  			abuflen = len;
-			posix_memalign((void**)&abuf, 4096, abuflen);
-			posix_memalign((void**)&bbuf, 4096, abuflen);
+			if(posix_memalign((void**)&abuf, 4096, abuflen))
+				fail("cannot allocate memory");
+			if(posix_memalign((void**)&bbuf, 4096, abuflen))
+				fail("cannot allocate memory");
  		}
  
  		lseek64(bfd, offset, 0);

@@ -1508,8 +1512,9 @@ static int child_grow(int afd, struct mdinfo *sra, unsigned long stripes,
  {
  	char *buf;
  	int degraded = 0;
+	int discard_result;
  
-	posix_memalign((void**)&buf, 4096, disks * chunk);
+	discard_result = posix_memalign((void**)&buf, 4096, disks * chunk);
  	sysfs_set_num(sra, NULL, "suspend_hi", 0);
  	sysfs_set_num(sra, NULL, "suspend_lo", 0);
  	grow_backup(sra, 0, stripes,

@@ -1536,8 +1541,9 @@ static int child_shrink(int afd, struct mdinfo *sra, unsigned long stripes,
  	unsigned long long start;
  	int rv;
  	int degraded = 0;
+	int discard_result;
  
-	posix_memalign((void**)&buf, 4096, disks * chunk);
+	discard_result = posix_memalign((void**)&buf, 4096, disks * chunk);
  	start = sra->component_size - stripes * chunk/512;
  	sysfs_set_num(sra, NULL, "sync_max", start);
  	sysfs_set_str(sra, NULL, "sync_action", "reshape");

@@ -1574,9 +1580,10 @@ static int child_same_size(int afd, struct mdinfo *sra, unsigned long stripes,
  	char *buf;
  	unsigned long long speed;
  	int degraded = 0;
+	int discard_result;
  
  
-	posix_memalign((void**)&buf, 4096, disks * chunk);
+	discard_result = posix_memalign((void**)&buf, 4096, disks * chunk);
  
  	sysfs_set_num(sra, NULL, "suspend_lo", 0);
  	sysfs_set_num(sra, NULL, "suspend_hi", 0);
Signature · 9 lines
1.7.0

-- 
Luca Berra -- [email protected]
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \
#7 lore raw
In reply to Luca Berra
Quoted 7 lines · On Sat, 27 Feb 2010 11:14:43 +0100 Luca Berra <[email protected]> wrote:

building current git with -O2 -Wall -Werror aborts with compiler warnings patch 1 fixes a strict-aliasing violation
patch 2 addresses trivial to fix warning
patch 3 is just _WRONG_: there is no clear way to bailout from the functions in Grow.c, and i have no idea if aborting would leave the array in an inconsistent state. For the time being I just decided to please gcc by storing the result somwere, but it is never checked.

Thanks.
I needed to add -D_FORTIFY_SOURCE=2 to get all those warnings as well, so I've told the Makefile about that.

Your patches were space-damaged somehow so I didn't apply them directly, but I have fixed up all the warnings, sometimes the same way that you did, soemthings differently.

Thanks.

NeilBrown

Quoted 15 lines

L.

Luca Berra (3):
fix gcc warnings about strict-aliasing rules
fix compiler warnings
workaround unused_results

  Grow.c     |   30 +++++++++++++++++++-----------
  mdmon.c    |    2 +-
  restripe.c |    2 +-
  util.c     |    4 ++--
  4 files changed, 23 insertions(+), 15 deletions(-)