[PATCH*2] mdadm works with uClibc from SVN

[PATCH*2] mdadm works with uClibc from SVN

[Nix]

For some time, mdadm's been dumping core on me in my uClibc-built
initramfs. As you might imagine this is somewhat frustrating, not least
since my root filesystem's in LVM on RAID. Half an hour ago I got around
to debugging this.

Imagine my surprise when I found that it was effectively guaranteed to
crash: map_dev() in util.c is stubbed out for uClibc builds, and
returns -1 at all times. That means that code in, among other places,
config.c:load_partitions() is guaranteed to segfault, which is a bit
tough if you're using the (sane) default `DEVICE partitions' in your
mdadm.conf.

As far as I can tell this is entirely because ftw() support is not
implemented in uClibc. But as of November 2005, it *is* implemented in
uClibc 0.9.29-to-be in SVN, which rumour has it is soon to be officially
released. (The ftw() implementation is currently a copy of that from
glibc, but knowing the uClibc folks it will shrink dramatically as time
goes by. :) )

There are two options here. Either the trivial but rather unhelpful
approach of, well, *telling* people that what they're building
isn't going to work:

diff -durN 2.5.1-orig/Makefile 2.5.1-patched/Makefile
--- 2.5.1-orig/Makefile	2006-06-20 22:49:56.000000000 +0100
+++ 2.5.1-patched/Makefile	2006-06-20 22:50:34.000000000 +0100
@@ -97,6 +97,7 @@
 mdadm.tcc : $(SRCS) mdadm.h
 	$(TCC) -o mdadm.tcc $(SRCS)
 
+# This doesn't work
 mdadm.uclibc : $(SRCS) mdadm.h
 	$(UCLIBC_GCC) -DUCLIBC -DHAVE_STDINT_H -o mdadm.uclibc $(SRCS) $(STATICSRC)
 
@@ -115,6 +116,7 @@
 	rm -f $(OBJS)
 	$(CC) $(LDFLAGS) $(ASSEMBLE_FLAGS) -static -DHAVE_STDINT_H -o mdassemble.static $(ASSEMBLE_SRCS) $(STATICSRC)
 
+# This doesn't work
 mdassemble.uclibc : $(ASSEMBLE_SRCS) mdadm.h
 	rm -f $(OJS)
 	$(UCLIBC_GCC) $(ASSEMBLE_FLAGS) -DUCLIBC -DHAVE_STDINT_H -static -o mdassemble.uclibc $(ASSEMBLE_SRCS) $(STATICSRC)
diff -durN 2.5.1-orig/mdadm.h 2.5.1-patched/mdadm.h
--- 2.5.1-orig/mdadm.h	2006-06-02 06:35:22.000000000 +0100
+++ 2.5.1-patched/mdadm.h	2006-06-20 22:50:55.000000000 +0100
@@ -344,6 +344,7 @@
 #endif
 
 #ifdef UCLIBC
+#error This is known not to work.
   struct FTW {};
 # define FTW_PHYS 1
 #else

Or the even-more-trivial but arguably far more useful approach of making
it work when possible (failing to compile with old uClibc because of the
absence of <ftw.h>, and working perfectly well with new uClibc):

diff -durN 2.5.1-orig/Makefile 2.5.1-patched/Makefile
--- 2.5.1-orig/Makefile	2006-06-20 22:49:56.000000000 +0100
+++ 2.5.1-patched/Makefile	2006-06-20 22:52:34.000000000 +0100
@@ -98,7 +98,7 @@
 	$(TCC) -o mdadm.tcc $(SRCS)
 
 mdadm.uclibc : $(SRCS) mdadm.h
-	$(UCLIBC_GCC) -DUCLIBC -DHAVE_STDINT_H -o mdadm.uclibc $(SRCS) $(STATICSRC)
+	$(UCLIBC_GCC) -DHAVE_STDINT_H -o mdadm.uclibc $(SRCS) $(STATICSRC)
 
 mdadm.klibc : $(SRCS) mdadm.h
 	rm -f $(OBJS) 
@@ -117,7 +117,7 @@
 
 mdassemble.uclibc : $(ASSEMBLE_SRCS) mdadm.h
 	rm -f $(OJS)
-	$(UCLIBC_GCC) $(ASSEMBLE_FLAGS) -DUCLIBC -DHAVE_STDINT_H -static -o mdassemble.uclibc $(ASSEMBLE_SRCS) $(STATICSRC)
+	$(UCLIBC_GCC) $(ASSEMBLE_FLAGS) -DHAVE_STDINT_H -static -o mdassemble.uclibc $(ASSEMBLE_SRCS) $(STATICSRC)
 
 # This doesn't work
 mdassemble.klibc : $(ASSEMBLE_SRCS) mdadm.h
diff -durN 2.5.1-orig/mdadm.h 2.5.1-patched/mdadm.h
--- 2.5.1-orig/mdadm.h	2006-06-02 06:35:22.000000000 +0100
+++ 2.5.1-patched/mdadm.h	2006-06-20 22:53:02.000000000 +0100
@@ -343,14 +343,9 @@
 struct stat64;
 #endif
 
-#ifdef UCLIBC
-  struct FTW {};
+#include <ftw.h>
+#ifdef __dietlibc__
 # define FTW_PHYS 1
-#else
-# include <ftw.h>
-# ifdef __dietlibc__
-#  define FTW_PHYS 1
-# endif
 #endif
 
 extern int add_dev(const char *name, const struct stat *stb, int flag, struct FTW *s);
diff -durN 2.5.1-orig/util.c 2.5.1-patched/util.c
--- 2.5.1-orig/util.c	2006-06-16 01:25:44.000000000 +0100
+++ 2.5.1-patched/util.c	2006-06-20 22:54:13.000000000 +0100
@@ -354,21 +354,6 @@
 } *devlist = NULL;
 int devlist_ready = 0;
 
-#ifdef UCLIBC
-int add_dev(const char *name, const struct stat *stb, int flag, struct FTW *s)
-{
-	return 0;
-}
-char *map_dev(int major, int minor, int create)
-{
-#if 0
-	fprintf(stderr, "Warning - fail to map %d,%d to a device name\n",
-		major, minor);
-#endif
-	return NULL;
-}
-#else
-
 #ifdef __dietlibc__
 int add_dev_1(const char *name, const struct stat *stb, int flag)
 {
@@ -467,8 +452,6 @@
 	return nonstd ? nonstd : std;
 }
 
-#endif
-
 unsigned long calc_csum(void *super, int bytes)
 {
 	unsigned long long newcsum = 0;


With this latter patch, mdadm works flawlessly with my uClibc,
svnversion r15342 from 2006-06-08, and probably works just as well with
all SVN releases after r13017, 2005-12-30.


(One final request: please turn on world-readability in your generated
tarballs ;) right now util.c and some others are readable only by
user. Thanks.)

-- 
`NB: Anyone suggesting that we should say "Tibibytes" instead of
 Terabytes there will be hunted down and brutally slain.
 That is all.' --- Matthew Wilcox

Re: [PATCH*2] mdadm works with uClibc from SVN

[Nix]

On 20 Jun 2006, nix@esperi.org.uk suggested tentatively:
> Imagine my surprise when I found that it was effectively guaranteed to
> crash: map_dev() in util.c is stubbed out for uClibc builds, and
> returns -1 at all times.

Um, that is, returns NULL. Obviously.

-- 
`NB: Anyone suggesting that we should say "Tibibytes" instead of
 Terabytes there will be hunted down and brutally slain.
 That is all.' --- Matthew Wilcox

Re: [PATCH*2] mdadm works with uClibc from SVN

[Nix]

On 20 Jun 2006, nix@esperi.org.uk prattled cheerily:
> For some time, mdadm's been dumping core on me in my uClibc-built
> initramfs. As you might imagine this is somewhat frustrating, not least
> since my root filesystem's in LVM on RAID. Half an hour ago I got around
> to debugging this.

Ping?

-- 
`NB: Anyone suggesting that we should say "Tibibytes" instead of
 Terabytes there will be hunted down and brutally slain.
 That is all.' --- Matthew Wilcox

Re: [PATCH*2] mdadm works with uClibc from SVN

[Neil Brown]

On Friday June 23, nix@esperi.org.uk wrote:
> On 20 Jun 2006, nix@esperi.org.uk prattled cheerily:
> > For some time, mdadm's been dumping core on me in my uClibc-built
> > initramfs. As you might imagine this is somewhat frustrating, not least
> > since my root filesystem's in LVM on RAID. Half an hour ago I got around
> > to debugging this.
> 
> Ping?

No, but I do know someone by that name.....

Yeh, it's on my todo list. 
I suspect I would rather make mdadm not dump core of ftw isn't
available.
Is there some #define in an include file which will allow me to tell
if the current uclibc supports ftw or not?

NeilBrown

(and I'm more like to reply to mail if it the "To" or "Cc" line
 mentions me specifically - it's less likely to get lost that way).

Re: [PATCH*2] mdadm works with uClibc from SVN

[Nix]

On Fri, 23 Jun 2006, Neil Brown mused:
> On Friday June 23, nix@esperi.org.uk wrote:
>> On 20 Jun 2006, nix@esperi.org.uk prattled cheerily:
>> > For some time, mdadm's been dumping core on me in my uClibc-built
>> > initramfs. As you might imagine this is somewhat frustrating, not least
>> > since my root filesystem's in LVM on RAID. Half an hour ago I got around
>> > to debugging this.
>> 
>> Ping?
> 
> No, but I do know someone by that name.....

Sorry, I'm pinging all the patches I've sent out in the last few weeks
and this one was on my hit list :)

> Yeh, it's on my todo list. 

Agreed.

> I suspect I would rather make mdadm not dump core of ftw isn't
> available.

It was... surprising. I'm still not sure how I ever made it work without
this fxi; the first uClibc-based initramfs I ever rolled had mdadm 2.3.1
in it and a `DEVICE partitions', and there was no core dump. A mystery.

> Is there some #define in an include file which will allow me to tell
> if the current uclibc supports ftw or not?

I misspoke: ftw was split into multiple files in late 2005, but it was
originally added in September 2003, in time for version 0.9.21.

Obviously the #defines in <ftw.h> don't exist before that date, but
that's a bit late to check, really.

<features.h> provides the macros __UCLIBC_MAJOR__, __UCLIBC_MINOR__, and
__UCLIBC_SUBLEVEL__: versions above 0.9.20 appear to support ftw()
(at least, they have the function, in 32-bit form at least, which
is certainly enough for this application!)

> (and I'm more like to reply to mail if it the "To" or "Cc" line
>  mentions me specifically - it's less likely to get lost that way).

OK.

-- 
`NB: Anyone suggesting that we should say "Tibibytes" instead of
 Terabytes there will be hunted down and brutally slain.
 That is all.' --- Matthew Wilcox

Re: [PATCH*2] mdadm works with uClibc from SVN

[Luca Berra]

On Fri, Jun 23, 2006 at 08:45:47PM +0100, Nix wrote:
>On Fri, 23 Jun 2006, Neil Brown mused:
>> Is there some #define in an include file which will allow me to tell
>> if the current uclibc supports ftw or not?

it is not only depending on the uClibc version, but also if ftw support
was compiled in or not.

>I misspoke: ftw was split into multiple files in late 2005, but it was
>originally added in September 2003, in time for version 0.9.21.
>
>Obviously the #defines in <ftw.h> don't exist before that date, but
>that's a bit late to check, really.
>
><features.h> provides the macros __UCLIBC_MAJOR__, __UCLIBC_MINOR__, and
>__UCLIBC_SUBLEVEL__: versions above 0.9.20 appear to support ftw()
>(at least, they have the function, in 32-bit form at least, which
>is certainly enough for this application!)

the following would be the correct check.

#include <features.h>
#ifdef __UCLIBC_HAS_FTW__
.....
#else
.....
#endif /* __UCLIBC_HAS_FTW__ */


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

Re: [PATCH*2] mdadm works with uClibc from SVN

[Nix]

On Sat, 24 Jun 2006, Luca Berra said:
> On Fri, Jun 23, 2006 at 08:45:47PM +0100, Nix wrote:
>>On Fri, 23 Jun 2006, Neil Brown mused:
>>> Is there some #define in an include file which will allow me to tell
>>> if the current uclibc supports ftw or not?
> 
> it is not only depending on the uClibc version, but also if ftw support
> was compiled in or not.

True.

>><features.h> provides the macros __UCLIBC_MAJOR__, __UCLIBC_MINOR__, and
>>__UCLIBC_SUBLEVEL__: versions above 0.9.20 appear to support ftw()
>>(at least, they have the function, in 32-bit form at least, which
>>is certainly enough for this application!)
> 
> the following would be the correct check.
> 
> #include <features.h>
> #ifdef __UCLIBC_HAS_FTW__

Gah. I looked *in that file* and *still* failed to spot that.

You are, of course, correct.

-- 
`NB: Anyone suggesting that we should say "Tibibytes" instead of
 Terabytes there will be hunted down and brutally slain.
 That is all.' --- Matthew Wilcox

Re: [PATCH*2] mdadm works with uClibc from SVN

[Neil Brown]

On Tuesday June 20, nix@esperi.org.uk wrote:
> For some time, mdadm's been dumping core on me in my uClibc-built
> initramfs. As you might imagine this is somewhat frustrating, not least
> since my root filesystem's in LVM on RAID. Half an hour ago I got around
> to debugging this.
> 
> Imagine my surprise when I found that it was effectively guaranteed to
> crash: map_dev() in util.c is stubbed out for uClibc builds, and
> returns -1 at all times. That means that code in, among other places,
> config.c:load_partitions() is guaranteed to segfault, which is a bit
> tough if you're using the (sane) default `DEVICE partitions' in your
> mdadm.conf.

I'm confused.  map_dev doesn't return -1, it returns NULL.
And the code in load_partitions() handles a NULL return.
So I cannot see why it would be core dumping.
If you have more details, I'd be really interested.

That said: utils doesn't handle the case for 'ftw not available' as
well as it could.  I will fix that.

NeilBrown

Re: [PATCH*2] mdadm works with uClibc from SVN

[Nix]

On 26 Jun 2006, Neil Brown said:
> On Tuesday June 20, nix@esperi.org.uk wrote:
>> For some time, mdadm's been dumping core on me in my uClibc-built
>> initramfs. As you might imagine this is somewhat frustrating, not least
>> since my root filesystem's in LVM on RAID. Half an hour ago I got around
>> to debugging this.
>> 
>> Imagine my surprise when I found that it was effectively guaranteed to
>> crash: map_dev() in util.c is stubbed out for uClibc builds, and
>> returns -1 at all times. That means that code in, among other places,
>> config.c:load_partitions() is guaranteed to segfault, which is a bit
>> tough if you're using the (sane) default `DEVICE partitions' in your
>> mdadm.conf.
> 
> I'm confused.  map_dev doesn't return -1, it returns NULL.

Yes, that was a typo on my part.

> And the code in load_partitions() handles a NULL return.

Er, what?

The code in load_partitions() in mdadm 2.5.1 and below says

,----[ config.c:load_partitions() ]
| name = map_dev(major, minor, 1);
| 
| d = malloc(sizeof(*d));
| d->devname = strdup(name);
`----

So if map_dev() returns NULL, we do a strdup(NULL) -> crash inside
uClibc.

> So I cannot see why it would be core dumping.
> If you have more details, I'd be really interested.

Backtrace (against uClibc compiled with debugging symbols, tested
against a temporary RAID array on a loopback device):

(gdb) run
Starting program: /usr/packages/mdadm/i686-loki/mdadm.uclibc --assemble --uuid=572aeae1:532641bf:49c9aec5:6036454d /dev/md127

Program received signal SIGSEGV, Segmentation fault.
0x080622f8 in strlen (s=0x0) at libc/string/i386/strlen.c:40
40      libc/string/i386/strlen.c: No such file or directory.
        in libc/string/i386/strlen.c
(gdb) bt
#0  0x080622f8 in strlen (s=0x0) at libc/string/i386/strlen.c:40
#1  0x08062d1d in strdup (s1=0x0) at libc/string/strdup.c:30
#2  0x0804b0df in load_partitions () at config.c:247
#3  0x0804b77c in conf_get_devs (conffile=0x0) at config.c:715
#4  0x0804d685 in Assemble (st=0x0, mddev=0xbf9c753e "/dev/md127", mdfd=6, ident=0xbf9c630c, conffile=0x0, devlist=0x0, backup_file=0x0, readonly=0, runstop=0, update=0x0, homehost=0x0,
    verbose=0, force=0) at Assemble.c:184
#5  0x08049d62 in main (argc=4, argv=0xbf9c6514) at mdadm.c:958
(gdb) frame 2
#2  0x0804b0df in load_partitions () at config.c:247
247                     d->devname = strdup(name);
(gdb) info locals
name = 0x0
mp = 0xbf9c5a34 "     0    9873360 hda\n"
f = (FILE *) 0x807b018
buf = "   3     0    9873360 hda\n", '\0' <repeats 546 times>, "1\000\000\000\000\000((ÿÿÿ\177\021", '\0' <repeats 15 times>, "\021\000\000\000\231\231\231\031Z]\234¿\000\000(\005ÿÿÿ\177¼\\\234¿ÓU\006\bZ]\234¿\000\000\000\000\n", '\0' <repeats 15 times>, "d^\234¿ÏÈ\004\bX]\234¿\000\000\000\000\n\000\000\000\000\000Linux", '\0' <repeats 60 times>, "loki\000)", '\0' <repeats 43 times>, "\006", '\0' <repeats 15 times>, "2.6.17-s\000\000\000\000à]\234¿ßÉ\005\b|]\234¿ð]\234¿", '\0' <repeats 12 times>...
rv = (mddev_dev_t) 0x0


I hope the bug's more obvious now :)

> That said: utils doesn't handle the case for 'ftw not available' as
> well as it could.  I will fix that.

The __UCLIBC_HAS_FTW__ macro in <features.h> is the right way to do that
for uClibc, as Luca noted.

-- 
`NB: Anyone suggesting that we should say "Tibibytes" instead of
 Terabytes there will be hunted down and brutally slain.
 That is all.' --- Matthew Wilcox
-
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH*2] mdadm works with uClibc from SVN

[Neil Brown]

On Tuesday June 27, nix@esperi.org.uk wrote:
> > And the code in load_partitions() handles a NULL return.
> 
> Er, what?
> 
> The code in load_partitions() in mdadm 2.5.1 and below says
> 
> ,----[ config.c:load_partitions() ]
> | name = map_dev(major, minor, 1);
> | 
> | d = malloc(sizeof(*d));
> | d->devname = strdup(name);
> `----
> 

Ahh.. uhmmm... Oh yes.  I've fixed that since, but completely forgot
about it and the change log didn't make it obvious.
 From
    http://neil.brown.name/git/?p=mdadm

           28 hours ago 	Neil Brown 	More consistent honoring of --configfile
           28 hours ago 	Neil Brown 	Fix user-after-free bug in error path in --monitor ...
           28 hours ago 	Neil Brown 	Improve ftw handling.
           31 hours ago 	Neil Brown 	Fix a recently introduced bug, and make --assemble ...
           31 hours ago 	Neil Brown 	Work around bug in --add handling for version-1 superb ...
           31 hours ago 	Neil Brown 	Add 'Array Slot' line to --examine for version-1 superb ...
           31 hours ago 	martin f krafft 	Fix manpage typo 
           31 hours ago 	Neil Brown 	Get the changelog uptodate for recent changes.
           7 days ago 	Paul Clements 	Move a variable declaration to the declaration area.
this one-> 7 days ago 	Neil Brown 	Stop map_dev from returning [0:0] 
           7 days ago 	Paul Clements 	Fix problem with post-increment usage in macro 
           11 days ago 	Neil Brown 	Release 2.5.1 

So: that's fixed for 2.5.2.  Thanks for following it up.

NeilBrown

Re: [PATCH*2] mdadm works with uClibc from SVN

[Nix]

On Tue, 27 Jun 2006, Neil Brown prattled cheerily:
> On Tuesday June 27, nix@esperi.org.uk wrote:
>> ,----[ config.c:load_partitions() ]
>> | name = map_dev(major, minor, 1);
>> | 
>> | d = malloc(sizeof(*d));
>> | d->devname = strdup(name);
>> `----
>> 
> 
> Ahh.. uhmmm... Oh yes.  I've fixed that since, but completely forgot
> about it and the change log didn't make it obvious.

Yahoo. That's what I like about free software: the pre-emptive bugfixes!
It seems that as soon as I find a bug, someone else will have already
fixed it :)

> So: that's fixed for 2.5.2.  Thanks for following it up.

No, thank *you* for fixing my boot process :)

-- 
`NB: Anyone suggesting that we should say "Tibibytes" instead of
 Terabytes there will be hunted down and brutally slain.
 That is all.' --- Matthew Wilcox