[mdadm PATCH 0/3] *** fix gcc warnings ***

[mdadm PATCH 0/3] *** fix gcc warnings ***

[Luca Berra]

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(-)


-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

[mdadm PATCH 2/3] fix compiler warnings

[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 <bluca@comedia.it>
---
   Grow.c     |    7 ++++---
   mdmon.c    |    2 +-
   restripe.c |    2 +-
   3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/Grow.c b/Grow.c
index 74b63b6..5806fc3 100644
--- a/Grow.c
+++ b/Grow.c
@@ -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);
   }
   
diff --git a/mdmon.c b/mdmon.c
index 31d45fd..70fbd0b 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -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)
diff --git a/restripe.c b/restripe.c
index f673206..d885e76 100644
--- a/restripe.c
+++ b/restripe.c
@@ -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)
-- 
1.7.0

-- 
Luca Berra -- bluca@comedia.it
          Communication Media & Services S.r.l.
   /"\
   \ /     ASCII RIBBON CAMPAIGN
    X        AGAINST HTML MAIL
   / \

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

[Luca Berra]

Signed-off-by: Luca Berra <bluca@comedia.it>
---
   util.c |    4 ++--
   1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/util.c b/util.c
index 68f048d..1def2a0 100644
--- a/util.c
+++ b/util.c
@@ -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;
   		}
-- 
1.7.0

-- 
Luca Berra -- bluca@comedia.it
          Communication Media & Services S.r.l.
   /"\
   \ /     ASCII RIBBON CAMPAIGN
    X        AGAINST HTML MAIL
   / \

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

[Michael Tokarev]

Luca Berra wrong:
> Signed-off-by: Luca Berra <bluca@comedia.it>
> ---
>   util.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/util.c b/util.c
> index 68f048d..1def2a0 100644
> --- a/util.c
> +++ b/util.c
> @@ -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?

>           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

>               if (curr_part_end > *endofpart)
>                   *endofpart = curr_part_end;
>           }

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

[Luca Berra]

On Tue, Mar 02, 2010 at 10:40:21AM +0300, Michael Tokarev wrote:
>Luca Berra wrong:
>> Signed-off-by: Luca Berra <bluca@comedia.it>
>> ---
>>   util.c |    4 ++--
>>   1 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/util.c b/util.c
>> index 68f048d..1def2a0 100644
>> --- a/util.c
>> +++ b/util.c
>> @@ -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.

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.

-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Neil Brown]

On Tue, 2 Mar 2010 08:48:11 +0100
Luca Berra <bluca@comedia.it> wrote:

> On Tue, Mar 02, 2010 at 10:40:21AM +0300, Michael Tokarev wrote:
> >Luca Berra wrong:
> >> Signed-off-by: Luca Berra <bluca@comedia.it>
> >> ---
> >>   util.c |    4 ++--
> >>   1 files changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/util.c b/util.c
> >> index 68f048d..1def2a0 100644
> >> --- a/util.c
> >> +++ b/util.c
> >> @@ -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.
> 
> 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

[mdadm PATCH 3/3] workaround unused results

[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 <bluca@comedia.it>
---
   Grow.c |   23 +++++++++++++++--------
   1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/Grow.c b/Grow.c
index 5806fc3..8dbe139 100644
--- a/Grow.c
+++ b/Grow.c
@@ -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);
-- 
1.7.0

-- 
Luca Berra -- bluca@comedia.it
          Communication Media & Services S.r.l.
   /"\
   \ /     ASCII RIBBON CAMPAIGN
    X        AGAINST HTML MAIL
   / \

Re: [mdadm PATCH 0/3] *** fix gcc warnings ***

[Neil Brown]

On Sat, 27 Feb 2010 11:14:43 +0100
Luca Berra <bluca@comedia.it> 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


> 
> 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(-)
> 
>