[PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[Luca Berra]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]

On Fri, May 26, 2006 at 04:33:08PM +1000, Neil Brown wrote:
>
>
>I am pleased to announce the availability of
>   mdadm version 2.5
>

hello,
i tried rebuilding mdadm 2.5 on current mandriva cooker, which uses
gcc-4.1.1, glibc-2.4 and dietlibc 0.29 and found the following issues
addressed by patches attacched to this message
I would be glad if you could review these patches and include them in
upcoming mdadm releases.

- mdadm-2.3.1-kernel-byteswap-include-fix.patch
reverts a change introduced with mdadm 2.3.1 for redhat compatibility
asm/byteorder.h is an architecture dependent file and does more
stuff than a call to the linux/byteorder/XXX_endian.h
the fact that not calling asm/byteorder.h does not define
__BYTEORDER_HAS_U64__ is just an example of issues that might arise.
if redhat is broken it should be worked around differently than breaking
mdadm.

- mdadm-2.4-snprintf.patch
this is self commenting, just an error in the snprintf call

- mdadm-2.4-strict-aliasing.patch
fix for another srict-aliasing problem, you can typecast a reference to a
void pointer to anything, you cannot typecast a reference to a struct.

- mdadm-2.5-mdassemble.patch
pass CFLAGS to mdassemble build, enabling -Wall -Werror showed some
issues also fixed by the patch.

- mdadm-2.5-rand.patch
Posix dictates rand() versus bsd random() function, and dietlibc
deprecated random(), so switch to srand()/rand() and make everybody
happy.

- mdadm-2.5-unused.patch
glibc 2.4 is pedantic on ignoring return values from fprintf, fwrite and
write, so now we check the rval and actually do something with it.
in the Grow.c case i only print a warning, since i don't think we can do
anithing in case we fail invalidating those superblocks (is should never
happen, but then...)

Regards,
L.


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

[-- Attachment #2: mdadm-2.3.1-kernel-byteswap-include-fix.patch --]
[-- Type: text/plain, Size: 802 bytes --]

* Sat Feb 18 2006 Christiaan Welvaart <cjw@daneel.dyndns.org>
not including asm/byteorder.h will not define __BYTEORDER_HAS_U64__
causing __fswab64 to be undefined and failure compiling mdadm on
big_endian architectures like PPC

--- mdadm-2.3.1/mdadm.h.bak	2006-02-06 04:52:12.000000000 +0100
+++ mdadm-2.3.1/mdadm.h	2006-02-18 03:51:59.786926267 +0100
@@ -72,16 +72,7 @@
 #include	"bitmap.h"
 
 #include <endian.h>
-/* #include "asm/byteorder.h" Redhat don't like this so... */
-#if __BYTE_ORDER == __LITTLE_ENDIAN
-#  include <linux/byteorder/little_endian.h>
-#elif __BYTE_ORDER == __BIG_ENDIAN
-#  include <linux/byteorder/big_endian.h>
-#elif __BYTE_ORDER == __PDP_ENDIAN
-#  include <linux/byteorder/pdp_endian.h>
-#else
-#  error "unknown endianness."
-#endif
+#include <asm/byteorder.h>
 
 
 

[-- Attachment #3: mdadm-2.4-snprintf.patch --]
[-- Type: text/plain, Size: 424 bytes --]

* Sat May 27 2006 Luca Berra <bluca@vodka.it>
snprintf size should be at most the size of the buffer

--- mdadm-2.4/util.c.snprintf	2006-05-27 13:53:18.000000000 +0200
+++ mdadm-2.4/util.c	2006-05-27 13:53:38.000000000 +0200
@@ -439,7 +439,7 @@
 		}
 	if (create && !std && !nonstd) {
 		static char buf[30];
-		snprintf(buf, 1024, "%d:%d", major, minor);
+		snprintf(buf, 30, "%d:%d", major, minor);
 		nonstd = buf;
 	}
 

[-- Attachment #4: mdadm-2.4-strict-aliasing.patch --]
[-- Type: text/plain, Size: 928 bytes --]

* Sat May 27 2006 Luca Berra <bluca@vodka.it>
This is to avoid gcc warnings when building with strict-aliasing optimization

--- mdadm-2.4/dlink.h.alias	2006-05-26 21:05:07.000000000 +0200
+++ mdadm-2.4/dlink.h	2006-05-27 12:32:58.000000000 +0200
@@ -4,16 +4,16 @@
 
 struct __dl_head
 {
-    struct __dl_head *	dh_prev;
-    struct __dl_head *	dh_next;
+    void * dh_prev;
+    void * dh_next;
 };
 
 #define	dl_alloc(size)	((void*)(((char*)calloc(1,(size)+sizeof(struct __dl_head)))+sizeof(struct __dl_head)))
 #define	dl_new(t)	((t*)dl_alloc(sizeof(t)))
 #define	dl_newv(t,n)	((t*)dl_alloc(sizeof(t)*n))
 
-#define dl_next(p) *((void**)&(((struct __dl_head*)(p))[-1].dh_next))
-#define dl_prev(p) *((void**)&(((struct __dl_head*)(p))[-1].dh_prev))
+#define dl_next(p) *(&(((struct __dl_head*)(p))[-1].dh_next))
+#define dl_prev(p) *(&(((struct __dl_head*)(p))[-1].dh_prev))
 
 void *dl_head(void);
 char *dl_strdup(char *);

[-- Attachment #5: mdadm-2.5-mdassemble.patch --]
[-- Type: text/plain, Size: 2062 bytes --]

* Sat May 27 2006 Luca Berra <bluca@vodka.it>
add CFLAGS to mdassemble build and fix a couple of non-returning functions

--- mdadm-2.5/mdadm.h.bluca	2006-05-27 14:25:53.000000000 +0200
+++ mdadm-2.5/mdadm.h	2006-05-27 15:20:37.000000000 +0200
@@ -44,10 +44,8 @@
 #include	<errno.h>
 #include	<string.h>
 #include	<syslog.h>
-#ifdef __dietlibc__NONO
-int strncmp(const char *s1, const char *s2, size_t n) __THROW __pure__;
-char *strncpy(char *dest, const char *src, size_t n) __THROW;
-#include    <strings.h>
+#ifdef __dietlibc__
+#include	<strings.h>
 #endif
 
 
--- mdadm-2.5/mdassemble.c.bluca	2006-05-27 15:11:02.000000000 +0200
+++ mdadm-2.5/mdassemble.c	2006-05-27 15:15:24.000000000 +0200
@@ -54,7 +54,7 @@
 };
 
 #ifndef MDASSEMBLE_AUTO
-/* from mdadm.c */
+/* from mdopen.c */
 int open_mddev(char *dev, int autof/*unused */)
 {
 	int mdfd = open(dev, O_RDWR, 0);
@@ -79,7 +79,7 @@
 int verbose = 0;
 int force = 0;
 
-int main() {
+int main(int argc, char *argv[]) {
 	mddev_ident_t array_list =  conf_get_ident(configfile, NULL);
 	if (!array_list) {
 		fprintf(stderr, Name ": No arrays found in config file\n");
@@ -100,4 +100,5 @@
 					   NULL, NULL,
 					   readonly, runstop, NULL, NULL, verbose, force);
 		}
+	return rv;
 }
--- mdadm-2.5/Makefile.bluca	2006-05-27 14:56:07.000000000 +0200
+++ mdadm-2.5/Makefile	2006-05-27 15:24:07.000000000 +0200
@@ -73,7 +73,7 @@
 	mdopen.c super0.c super1.c bitmap.c restripe.c sysfs.c
 
 ASSEMBLE_SRCS := mdassemble.c Assemble.c config.c dlink.c util.c super0.c super1.c
-ASSEMBLE_FLAGS:= -DMDASSEMBLE
+ASSEMBLE_FLAGS:= $(CFLAGS) -DMDASSEMBLE
 ifdef MDASSEMBLE_AUTO
 ASSEMBLE_SRCS += mdopen.c mdstat.c
 ASSEMBLE_FLAGS += -DMDASSEMBLE_AUTO
--- mdadm-2.5/util.c.bluca	2006-05-27 14:25:53.000000000 +0200
+++ mdadm-2.5/util.c	2006-05-27 15:34:47.000000000 +0200
@@ -375,7 +375,7 @@
 }
 int nftw(const char *path, int (*han)(const char *name, const struct stat *stb, int flag, struct FTW *s), int nopenfd, int flags)
 {
-	ftw(path, add_dev_1, nopenfd);
+	return ftw(path, add_dev_1, nopenfd);
 }
 #endif
 

[-- Attachment #6: mdadm-2.5-rand.patch --]
[-- Type: text/plain, Size: 2845 bytes --]

* Sat May 27 2006 Luca Berra <bluca@vodka.it>
POSIX dictates rand/srand instead of BSD srandom/random

--- mdadm-2.5/Assemble.c.rand	2006-05-27 14:40:30.000000000 +0200
+++ mdadm-2.5/Assemble.c	2006-05-27 15:32:39.000000000 +0200
@@ -412,10 +412,10 @@
 				int rfd;
 				if ((rfd = open("/dev/urandom", O_RDONLY)) < 0 ||
 				    read(rfd, ident->uuid, 16) != 16) {
-					*(__u32*)(ident->uuid) = random();
-					*(__u32*)(ident->uuid+1) = random();
-					*(__u32*)(ident->uuid+2) = random();
-					*(__u32*)(ident->uuid+3) = random();
+					*(__u32*)(ident->uuid) = rand();
+					*(__u32*)(ident->uuid+1) = rand();
+					*(__u32*)(ident->uuid+2) = rand();
+					*(__u32*)(ident->uuid+3) = rand();
 				}
 				if (rfd >= 0) close(rfd);
 			}
--- mdadm-2.5/mdadm.c.rand	2006-05-26 07:43:35.000000000 +0200
+++ mdadm-2.5/mdadm.c	2006-05-27 15:31:18.000000000 +0200
@@ -107,7 +107,7 @@
 
 	int mdfd = -1;
 
-	srandom(time(0) ^ getpid());
+	srand(time(0) ^ getpid());
 
 	ident.uuid_set=0;
 	ident.level = UnSet;
--- mdadm-2.5/super0.c.rand	2006-05-27 14:34:54.000000000 +0200
+++ mdadm-2.5/super0.c	2006-05-27 15:33:08.000000000 +0200
@@ -539,7 +539,7 @@
 	sb->patch_version = 0;
 	sb->gvalid_words = 0; /* ignored */
 	if (rfd < 0 || read(rfd, &sb->set_uuid0, 4) != 4)
-		sb->set_uuid0 = random();
+		sb->set_uuid0 = rand();
 	sb->ctime = time(0);
 	sb->level = info->level;
 	if (size != info->size)
@@ -550,9 +550,9 @@
 	sb->md_minor = info->md_minor;
 	sb->not_persistent = 0;
 	if (rfd < 0 || read(rfd, &sb->set_uuid1, 12) != 12) {
-		sb->set_uuid1 = random();
-		sb->set_uuid2 = random();
-		sb->set_uuid3 = random();
+		sb->set_uuid1 = rand();
+		sb->set_uuid2 = rand();
+		sb->set_uuid3 = rand();
 	}
 	if (rfd >= 0)
 		close(rfd);
--- mdadm-2.5/super1.c.rand	2006-05-27 14:37:22.000000000 +0200
+++ mdadm-2.5/super1.c	2006-05-27 15:33:29.000000000 +0200
@@ -583,10 +583,10 @@
 
 	if ((rfd = open("/dev/urandom", O_RDONLY)) < 0 ||
 	    read(rfd, sb->set_uuid, 16) != 16) {
-		*(__u32*)(sb->set_uuid) = random();
-		*(__u32*)(sb->set_uuid+4) = random();
-		*(__u32*)(sb->set_uuid+8) = random();
-		*(__u32*)(sb->set_uuid+12) = random();
+		*(__u32*)(sb->set_uuid) = rand();
+		*(__u32*)(sb->set_uuid+4) = rand();
+		*(__u32*)(sb->set_uuid+8) = rand();
+		*(__u32*)(sb->set_uuid+12) = rand();
 	}
 	if (rfd >= 0) close(rfd);
 
@@ -751,10 +751,10 @@
 
 	if ((rfd = open("/dev/urandom", O_RDONLY)) < 0 ||
 	    read(rfd, sb->device_uuid, 16) != 16) {
-		*(__u32*)(sb->device_uuid) = random();
-		*(__u32*)(sb->device_uuid+4) = random();
-		*(__u32*)(sb->device_uuid+8) = random();
-		*(__u32*)(sb->device_uuid+12) = random();
+		*(__u32*)(sb->device_uuid) = rand();
+		*(__u32*)(sb->device_uuid+4) = rand();
+		*(__u32*)(sb->device_uuid+8) = rand();
+		*(__u32*)(sb->device_uuid+12) = rand();
 	}
 	if (rfd >= 0) close(rfd);
 	sb->events = 0;

[-- Attachment #7: mdadm-2.5-unused.patch --]
[-- Type: text/plain, Size: 3471 bytes --]

* Sat May 27 2006 Luca Berra <bluca@vodka.it>
check return status of all write/fwrite functions as required by glibc 2.4

--- mdadm-2.5/Monitor.c.unused	2006-05-27 14:28:23.000000000 +0200
+++ mdadm-2.5/Monitor.c	2006-05-27 14:28:28.000000000 +0200
@@ -521,7 +521,7 @@
 				int n;
 				fprintf(mp, "\nP.S. The /proc/mdstat file current contains the following:\n\n");
 				while ( (n=fread(buf, 1, sizeof(buf), mdstat)) > 0)
-					fwrite(buf, 1, n, mp);
+					n=fwrite(buf, 1, n, mp); /* yes, i don't care about the result */
 				fclose(mdstat);
 			}
 			fclose(mp);
--- mdadm-2.5/super1.c.unused	2006-05-27 14:36:13.000000000 +0200
+++ mdadm-2.5/super1.c	2006-05-27 14:37:22.000000000 +0200
@@ -715,7 +715,8 @@
 			(((char*)sb)+1024);
 		if (__le32_to_cpu(bm->magic) == BITMAP_MAGIC) {
 			locate_bitmap1(st, fd, sbv);
-			write(fd, bm, sizeof(*bm));
+			if (write(fd, bm, sizeof(*bm)) != sizeof(*bm))
+			    return 5;
 		}
 	}
 	fsync(fd);
--- mdadm-2.5/super0.c.unused	2006-05-27 14:31:33.000000000 +0200
+++ mdadm-2.5/super0.c	2006-05-27 14:34:54.000000000 +0200
@@ -625,7 +625,8 @@
 	if (super->state & (1<<MD_SB_BITMAP_PRESENT)) {
 		struct bitmap_super_s * bm = (struct bitmap_super_s*)(super+1);
 		if (__le32_to_cpu(bm->magic) == BITMAP_MAGIC)
-			write(fd, bm, sizeof(*bm));
+			if (write(fd, bm, sizeof(*bm)) != sizeof(*bm))
+			    return 5;
 	}
 
 	fsync(fd);
--- mdadm-2.5/Grow.c.unused	2006-05-23 06:34:37.000000000 +0200
+++ mdadm-2.5/Grow.c	2006-05-27 14:25:53.000000000 +0200
@@ -801,7 +801,10 @@
 		memset(&bsb, 0, sizeof(bsb));
 		for (i=odisks; i<d ; i++) {
 			lseek64(fdlist[i], (offsets[i]+last_block)<<9, 0);
-			write(fdlist[i], &bsb, sizeof(bsb));
+			if (write(fdlist[i], &bsb, sizeof(bsb)) < 0) {
+				fprintf(stderr, Name ": %s: failed to invalidate metadata for raid disk %d\n",
+					devname, i);
+			}
 		}
 
 		/* unsuspend. */
--- mdadm-2.5/bitmap.c.unused	2006-05-19 09:15:32.000000000 +0200
+++ mdadm-2.5/bitmap.c	2006-05-27 14:52:31.000000000 +0200
@@ -399,16 +399,22 @@
 	return rv;
 }
 
-void bitmap_update_uuid(int fd, int *uuid)
+int bitmap_update_uuid(int fd, int *uuid)
 {
 	struct bitmap_super_s bm;
-	lseek(fd, 0, 0);
+	if (lseek(fd, 0, 0) != 0)
+		return 1;
 	if (read(fd, &bm, sizeof(bm)) != sizeof(bm))
-		return;
+		return 1;
 	if (bm.magic != __cpu_to_le32(BITMAP_MAGIC))
-		return;
+		return 1;
 	memcpy(bm.uuid, uuid, 16);
+	if (lseek(fd, 0, 0) != 0)
+		return 2;
+	if (write(fd, &bm, sizeof(bm)) != sizeof(bm)) {
+		lseek(fd, 0, 0);
+		return 2;
+	}
 	lseek(fd, 0, 0);
-	write(fd, &bm, sizeof(bm));
-	lseek(fd, 0, 0);
+	return 0;
 }
--- mdadm-2.5/mdadm.h.unused	2006-05-27 14:25:53.000000000 +0200
+++ mdadm-2.5/mdadm.h	2006-05-27 14:51:53.000000000 +0200
@@ -371,7 +371,7 @@
 			unsigned long long array_size,
 			int major);
 extern int ExamineBitmap(char *filename, int brief, struct supertype *st);
-extern void bitmap_update_uuid(int fd, int *uuid);
+extern int bitmap_update_uuid(int fd, int *uuid);
 
 extern int md_get_version(int fd);
 extern int get_linux_version(void);
--- mdadm-2.5/Assemble.c.unused	2006-05-26 08:28:07.000000000 +0200
+++ mdadm-2.5/Assemble.c	2006-05-27 14:40:30.000000000 +0200
@@ -451,7 +451,9 @@
 
 			if (strcmp(update, "uuid")==0 &&
 			    ident->bitmap_fd)
-				bitmap_update_uuid(ident->bitmap_fd, info.uuid);
+				if (bitmap_update_uuid(ident->bitmap_fd, info.uuid) != 0)
+					fprintf(stderr, Name ": Could not update uuid on %s.\n",
+						devname);
 		} else
 #endif
 		{

Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[dean gaudet]

On Sun, 28 May 2006, Luca Berra wrote:

> - mdadm-2.5-rand.patch
> Posix dictates rand() versus bsd random() function, and dietlibc
> deprecated random(), so switch to srand()/rand() and make everybody
> happy.

fwiw... lots of rand()s tend to suck... and RAND_MAX may not be large 
enough for this use.  glibc rand() is the same as random().  do you know 
if dietlibc's rand() is good enough?

-dean

Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[Luca Berra]

On Sun, May 28, 2006 at 10:08:19AM -0700, dean gaudet wrote:
>On Sun, 28 May 2006, Luca Berra wrote:
>
>> - mdadm-2.5-rand.patch
>> Posix dictates rand() versus bsd random() function, and dietlibc
>> deprecated random(), so switch to srand()/rand() and make everybody
>> happy.
>
>fwiw... lots of rand()s tend to suck... and RAND_MAX may not be large 
>enough for this use.  glibc rand() is the same as random().  do you know 
the fact that glibc rand() is the same implementation as random() was one of the
reason i believe we could switch to rand()

>if dietlibc's rand() is good enough?
dietlibc rand() and random() are the same function.
but random will throw a warning saying it is deprecated.

L.

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

Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[dean gaudet]

On Sun, 28 May 2006, Luca Berra wrote:

> dietlibc rand() and random() are the same function.
> but random will throw a warning saying it is deprecated.

that's terribly obnoxious... it's never going to be deprecated, there are 
only approximately a bazillion programs using random().

-dean

Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[Neil Brown]

On Sunday May 28, bluca@comedia.it wrote:
> On Fri, May 26, 2006 at 04:33:08PM +1000, Neil Brown wrote:
> >
> >
> >I am pleased to announce the availability of
> >   mdadm version 2.5
> >
> 
> hello,
> i tried rebuilding mdadm 2.5 on current mandriva cooker, which uses
> gcc-4.1.1, glibc-2.4 and dietlibc 0.29 and found the following issues
> addressed by patches attacched to this message
> I would be glad if you could review these patches and include them in
> upcoming mdadm releases.

Thanks for the patches.  They are greatly appreciated.

> 
> - mdadm-2.3.1-kernel-byteswap-include-fix.patch
> reverts a change introduced with mdadm 2.3.1 for redhat compatibility
> asm/byteorder.h is an architecture dependent file and does more
> stuff than a call to the linux/byteorder/XXX_endian.h
> the fact that not calling asm/byteorder.h does not define
> __BYTEORDER_HAS_U64__ is just an example of issues that might arise.
> if redhat is broken it should be worked around differently than breaking
> mdadm.

I don't understand the problem here.  What exactly breaks with the
code currently in 2.5?  mdadm doesn't need __BYTEORDER_HAS_U64__, so
why does not having id defined break anything?
The coomment from the patch says:
 > not including asm/byteorder.h will not define __BYTEORDER_HAS_U64__
 > causing __fswab64 to be undefined and failure compiling mdadm on
 > big_endian architectures like PPC

But again, mdadm doesn't use __fswab64 ....
More details please.

> 
> - mdadm-2.4-snprintf.patch
> this is self commenting, just an error in the snprintf call

I wonder how that snuck in...
There was an odd extra tab in the patch, but no-matter.
I changed it to use 'sizeof(buf)' to be consistent with other uses
of snprint.  Thanks.

> 
> - mdadm-2.4-strict-aliasing.patch
> fix for another srict-aliasing problem, you can typecast a reference to a
> void pointer to anything, you cannot typecast a reference to a
> struct.

Why can't I typecast a reference to a struct??? It seems very
unfair...
However I have no problem with the patch.  Applied.  Thanks.
I should really change it to use 'list.h' type lists from the linux
kernel.

> 
> - mdadm-2.5-mdassemble.patch
> pass CFLAGS to mdassemble build, enabling -Wall -Werror showed some
> issues also fixed by the patch.

yep, thanks.

> 
> - mdadm-2.5-rand.patch
> Posix dictates rand() versus bsd random() function, and dietlibc
> deprecated random(), so switch to srand()/rand() and make everybody
> happy.

Everybody?
'man 3 rand' tells me:

       Do not use this function in applications  intended  to  be
       portable when good randomness is needed.

Admittedly mdadm doesn't need to be portable - it only needs to run on
Linux.  But this line in the man page bothers me.

I guess
    -Drandom=rand -Dsrandom=srand
might work.... no.  stdlib.h doesn't like that.
'random' returns 'long int' while rand returns 'int'.
Interestingly 'random_r' returns 'int' as does 'rand_r'.

#ifdef __dietlibc__
#include	<strings.h>
/* dietlibc has deprecated random and srandom!! */
#define random rand
#define srandom srand
#endif

in mdadm.h.  Will that do you?


> 
> - mdadm-2.5-unused.patch
> glibc 2.4 is pedantic on ignoring return values from fprintf, fwrite and
> write, so now we check the rval and actually do something with it.
> in the Grow.c case i only print a warning, since i don't think we can do
> anithing in case we fail invalidating those superblocks (is should never
> happen, but then...)

Ok, thanks.


You can see these patches at
   http://neil.brown.name/cgi-bin/gitweb.cgi?p=mdadm

more welcome :-)

Thanks again,
NeilBrown

Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[Luca Berra]

On Mon, May 29, 2006 at 12:08:25PM +1000, Neil Brown wrote:
>On Sunday May 28, bluca@comedia.it wrote:
>Thanks for the patches.  They are greatly appreciated.
You're welcome
>> 
>> - mdadm-2.3.1-kernel-byteswap-include-fix.patch
>> reverts a change introduced with mdadm 2.3.1 for redhat compatibility
>> asm/byteorder.h is an architecture dependent file and does more
>> stuff than a call to the linux/byteorder/XXX_endian.h
>> the fact that not calling asm/byteorder.h does not define
>> __BYTEORDER_HAS_U64__ is just an example of issues that might arise.
>> if redhat is broken it should be worked around differently than breaking
>> mdadm.
>
>I don't understand the problem here.  What exactly breaks with the
>code currently in 2.5?  mdadm doesn't need __BYTEORDER_HAS_U64__, so
>why does not having id defined break anything?
>The coomment from the patch says:
> > not including asm/byteorder.h will not define __BYTEORDER_HAS_U64__
> > causing __fswab64 to be undefined and failure compiling mdadm on
> > big_endian architectures like PPC
>
>But again, mdadm doesn't use __fswab64 ....
>More details please.
you use __cpu_to_le64 (ie in super0.c line 987)

        bms->sync_size = __cpu_to_le64(size);

which in byteorder/big_endian.h is defined as

#define __cpu_to_le64(x) ((__force __le64)__swab64((x)))

but __swab64 is defined in byteorder/swab.h (included by
byteorder/big_endian.h) as

#if defined(__GNUC__) && (__GNUC__ >= 2) && defined(__OPTIMIZE__)
#  define __swab64(x) \
(__builtin_constant_p((__u64)(x)) ? \
___swab64((x)) : \
__fswab64((x)))
#else
#  define __swab64(x) __fswab64(x)
#endif /* OPTIMIZE */

and __fswab64 is defined further into byteorder/swab.h as

#ifdef __BYTEORDER_HAS_U64__
static __inline__ __attribute_const__ __u64 __fswab64(__u64 x)
.....
#endif /* __BYTEORDER_HAS_U64__ */

so building mdadm on a ppc (or i suppose a sparc) will break


now, if you look at /usr/src/linux/asm-*/byteorder.h you will notice
they are very different files, this makes me believe it is not a good
idea bypassing asm/byteorder.h
And no, just defining __BYTEORDER_HAS_U64__ will break on 32bit
big-endian cpus (and if i do not misread it might just compile and give
incorrect results)

>> 
>> - mdadm-2.4-snprintf.patch
>> this is self commenting, just an error in the snprintf call
>
>I wonder how that snuck in...
>There was an odd extra tab in the patch, but no-matter.
>I changed it to use 'sizeof(buf)' to be consistent with other uses
>of snprint.  Thanks.
yes, that would be better.

>> - mdadm-2.4-strict-aliasing.patch
>> fix for another srict-aliasing problem, you can typecast a reference to a
>> void pointer to anything, you cannot typecast a reference to a
>> struct.
>
>Why can't I typecast a reference to a struct??? It seems very
>unfair...
that's strict-aliasing optimization for you, i do agree it _is_ unfair.

>However I have no problem with the patch.  Applied.  Thanks.
>I should really change it to use 'list.h' type lists from the linux
>kernel.
hopefull redhat would not object :)

>> - mdadm-2.5-mdassemble.patch
>> pass CFLAGS to mdassemble build, enabling -Wall -Werror showed some
>> issues also fixed by the patch.
>
>yep, thanks.
>
>> 
>> - mdadm-2.5-rand.patch
>> Posix dictates rand() versus bsd random() function, and dietlibc
>> deprecated random(), so switch to srand()/rand() and make everybody
>> happy.
>
>Everybody?
>'man 3 rand' tells me:
>
>       Do not use this function in applications  intended  to  be
>       portable when good randomness is needed.
>
>Admittedly mdadm doesn't need to be portable - it only needs to run on
>Linux.  But this line in the man page bothers me.
>
>I guess
>    -Drandom=rand -Dsrandom=srand
>might work.... no.  stdlib.h doesn't like that.
>'random' returns 'long int' while rand returns 'int'.
>Interestingly 'random_r' returns 'int' as does 'rand_r'.
>
>#ifdef __dietlibc__
>#include	<strings.h>
>/* dietlibc has deprecated random and srandom!! */
>#define random rand
>#define srandom srand
>#endif
>
>in mdadm.h.  Will that do you?
>

yes, mdassemble will build, so it is ok for me.


>> 
>> - mdadm-2.5-unused.patch
>> glibc 2.4 is pedantic on ignoring return values from fprintf, fwrite and
>> write, so now we check the rval and actually do something with it.
>> in the Grow.c case i only print a warning, since i don't think we can do
>> anithing in case we fail invalidating those superblocks (is should never
>> happen, but then...)
>
>Ok, thanks.
>
>
>You can see these patches at
>   http://neil.brown.name/cgi-bin/gitweb.cgi?p=mdadm

Thanks.


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

Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[Neil Brown]

On Monday May 29, bluca@comedia.it wrote:
> On Mon, May 29, 2006 at 12:08:25PM +1000, Neil Brown wrote:
> >On Sunday May 28, bluca@comedia.it wrote:
> >Thanks for the patches.  They are greatly appreciated.
> You're welcome
> >> 
> >> - mdadm-2.3.1-kernel-byteswap-include-fix.patch
> >> reverts a change introduced with mdadm 2.3.1 for redhat compatibility
> >> asm/byteorder.h is an architecture dependent file and does more
> >> stuff than a call to the linux/byteorder/XXX_endian.h
> >> the fact that not calling asm/byteorder.h does not define
> >> __BYTEORDER_HAS_U64__ is just an example of issues that might arise.
> >> if redhat is broken it should be worked around differently than breaking
> >> mdadm.
> >
> >I don't understand the problem here.  What exactly breaks with the
> >code currently in 2.5?  mdadm doesn't need __BYTEORDER_HAS_U64__, so
> >why does not having id defined break anything?
> >The coomment from the patch says:
> > > not including asm/byteorder.h will not define __BYTEORDER_HAS_U64__
> > > causing __fswab64 to be undefined and failure compiling mdadm on
> > > big_endian architectures like PPC
> >
> >But again, mdadm doesn't use __fswab64 ....
> >More details please.
> you use __cpu_to_le64 (ie in super0.c line 987)
> 
>         bms->sync_size = __cpu_to_le64(size);
> 
> which in byteorder/big_endian.h is defined as
> 
> #define __cpu_to_le64(x) ((__force __le64)__swab64((x)))
> 
> but __swab64 is defined in byteorder/swab.h (included by
> byteorder/big_endian.h) as
> 
> #if defined(__GNUC__) && (__GNUC__ >= 2) && defined(__OPTIMIZE__)
> #  define __swab64(x) \
> (__builtin_constant_p((__u64)(x)) ? \
> ___swab64((x)) : \
> __fswab64((x)))
> #else
> #  define __swab64(x) __fswab64(x)
> #endif /* OPTIMIZE */


Grrr......

Thanks for the details.  I think I'll just give up and do it myself.
e.g.
short swap16(short in)
{
	int i;
	short out=0;
	for (i=0; i<4; i++) {
		out = out<<8 | (in&255);
		in = in >> 8;
	}
	return out;
}

I don't need top performance and at least this should be portable...

NeilBrown

Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[Alex Davis]

>short swap16(short in)
>{
>        int i;
>        short out=0;
>        for (i=0; i<4; i++) {
>                out = out<<8 | (in&255);
>                in = in >> 8;
>        }
>        return out;
>}

Shouldn't that be "for (i=0; i<2; i++) {..." ? 

I code, therefore I am

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[Eyal Lebedinsky]

Alex Davis wrote:
>>short swap16(short in)
>>{
>>       int i;
>>       short out=0;
>>       for (i=0; i<4; i++) {
>>               out = out<<8 | (in&255);
>>               in = in >> 8;
>>       }
>>       return out;
>>}
> 
> Shouldn't that be "for (i=0; i<2; i++) {..." ? 

In which case, do we really need this complexity rather than
a clear swap, e.g.
	return (short)(((in&0x0ff)<<8) | ((in>>8)&0x0ff))

Simple enough for a macro too.

-- 
Eyal Lebedinsky (eyal@eyal.emu.id.au) <http://samba.org/eyal/>
	attach .zip as .dat

Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[Neil Brown]

On Thursday June 1, eyal@eyal.emu.id.au wrote:
> Alex Davis wrote:
> >>short swap16(short in)
> >>{
> >>       int i;
> >>       short out=0;
> >>       for (i=0; i<4; i++) {
> >>               out = out<<8 | (in&255);
> >>               in = in >> 8;
> >>       }
> >>       return out;
> >>}
> > 
> > Shouldn't that be "for (i=0; i<2; i++) {..." ? 

Well, yes... if you want the code to actually work, I guess that might
be an improvement....


> 
> In which case, do we really need this complexity rather than
> a clear swap, e.g.
> 	return (short)(((in&0x0ff)<<8) | ((in>>8)&0x0ff))

What I like about the above is that the difference between swap16,
swap32 and swap64 is just one number.  In this case I think clarity
and maintainability are much more important than efficiency.

Thanks for the comment,
NeilBrown

Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)

[Luca Berra]

On Thu, Jun 01, 2006 at 11:22:09AM +1000, Neil Brown wrote:
>On Thursday June 1, eyal@eyal.emu.id.au wrote:
>> Alex Davis wrote:
>> >>short swap16(short in)
>> >>{
>> >>       int i;
>> >>       short out=0;
>> >>       for (i=0; i<4; i++) {
>> >>               out = out<<8 | (in&255);
>> >>               in = in >> 8;
>> >>       }
>> >>       return out;
>> >>}
>> > 
>> > Shouldn't that be "for (i=0; i<2; i++) {..." ? 
>
>Well, yes... if you want the code to actually work, I guess that might
>be an improvement....
most important,
shouldn't that be:
__u16 swap16(__u16 in)

?

Regards,
L.

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