Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
On Thu, Jun 01, 2006 at 11:22:09AM +1000, Neil Brown wrote:
On Thursday June 1, [EMAIL PROTECTED] 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 -- [EMAIL PROTECTED]
Communication Media & Services S.r.l.
/"\
\ / ASCII RIBBON CAMPAIGN
XAGAINST HTML MAIL
/ \
-
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
On Thursday June 1, [EMAIL PROTECTED] 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
-
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
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 ([EMAIL PROTECTED])
Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
>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
-
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
On Monday May 29, [EMAIL PROTECTED] wrote:
> On Mon, May 29, 2006 at 12:08:25PM +1000, Neil Brown wrote:
> >On Sunday May 28, [EMAIL PROTECTED] 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
-
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
On Mon, May 29, 2006 at 12:08:25PM +1000, Neil Brown wrote: On Sunday May 28, [EMAIL PROTECTED] 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 /* 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 -- [EMAIL PROTECTED] Communication Media & Services S.r.l. /"\ \ / ASCII RIBBON CAMPAIGN XAGAINST HTML MAIL / \ - To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
On Sunday May 28, [EMAIL PROTECTED] 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 /* 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 - To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
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 - To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
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 -- [EMAIL PROTECTED] Communication Media & Services S.r.l. /"\ \ / ASCII RIBBON CAMPAIGN XAGAINST HTML MAIL / \ - To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
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 - To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mdadm 2.5 (Was: ANNOUNCE: mdadm 2.5 - A tool for managing Soft RAID under Linux)
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 -- [EMAIL PROTECTED]
Communication Media & Services S.r.l.
/"\
\ / ASCII RIBBON CAMPAIGN
XAGAINST HTML MAIL
/ \
* Sat Feb 18 2006 Christiaan Welvaart <[EMAIL PROTECTED]>
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.0 +0100
+++ mdadm-2.3.1/mdadm.h 2006-02-18 03:51:59.786926267 +0100
@@ -72,16 +72,7 @@
#include "bitmap.h"
#include
-/* #include "asm/byteorder.h" Redhat don't like this so... */
-#if __BYTE_ORDER == __LITTLE_ENDIAN
-# include
-#elif __BYTE_ORDER == __BIG_ENDIAN
-# include
-#elif __BYTE_ORDER == __PDP_ENDIAN
-# include
-#else
-# error "unknown endianness."
-#endif
+#include
* Sat May 27 2006 Luca Berra <[EMAIL PROTECTED]>
snprintf size should be at most the size of the buffer
--- mdadm-2.4/util.c.snprintf 2006-05-27 13:53:18.0 +0200
+++ mdadm-2.4/util.c2006-05-27 13:53:38.0 +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;
}
* Sat May 27 2006 Luca Berra <[EMAIL PROTECTED]>
This is to avoid gcc warnings when building with strict-aliasing optimization
--- mdadm-2.4/dlink.h.alias 2006-05-26 21:05:07.0 +0200
+++ mdadm-2.4/dlink.h 2006-05-27 12:32:58.0 +0200
@@ -4,16 +4,16 @@
struct __dl_head
{
-struct __dl_head * dh_prev;
-struct __dl_head * dh_next;
+void * dh_prev;
+void * dh_next;
};
#definedl_alloc(size) ((void*)(((char*)calloc(1,(size)+sizeof(struct
__dl_head)))+sizeof(struct __dl_head)))
#definedl_new(t) ((t*)dl_alloc(sizeof(t)))
#definedl_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 *);
* Sat May 27 2006 Luca Berra <[EMAIL PROTECTED]>
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.0 +0200
+++ mdadm-2.5/mdadm.h 2006-05-27 15:20:37.0 +0200
@@ -44,10 +44,8 @@
#include
#include
#include
-#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
+#ifdef __dietlibc__
+#include
#endif
--- mdadm-2.5/mdassemble.c.bluca2006-05-27 15:11:02.0 +0200
+++ mdadm-2.5/mdassemble.c 2006-05-27 15:15:24.0 +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;
in
