md= broken. Found problem. Can't fix it. : (

I have multiple Linux hosts on a SAN, making autodetect of raid devices dangerous. This problem should be solved by specing an 'md=' for each device on the cmdline, but unfortuantly it's broken.

Between a few emails to mingo and several wasted hours, I've managed to figure out the problem. However I don't know how to fix it; it *should* be working from what I can see.

My only guess right now is because I'm using gcc 2.95.2, and it's doing something funky. (And I do not have another version to test with right now.)

The problem is during parsing of the md= line, name_to_kdev_t() is not returning the proper k_dev_t for the device. (IE /dev/sdd5 returns as 16:45 /dev/sde5 returns as 20:05.)

However if I pass static text to name_to_kdev_t(), it works. I first believed it was in how the str pointer was sent to name_to_kdev_t(), (running over into comma's, instead of seperate terminated strings) I
fixed that but the problem persists.

My current test for loop is attached. The hard coded device names printk out to a proper major:minor. The devicenames obtained from 'str' don't. I don't see the bug in here or name_to_kdev_t()...

I'm testing this with the following cmdline:
root=/dev/md0 raid=noautodetect md=0,/dev/hdd5,/dev/hde5 md=1,/dev/hdd6,/dev/hde6 md=2,/dev/hdd7,/dev/hde7 md=3,/dev/hdd8,/dev/hde8 mem=524288K

"Nobody will ever be safe until the last cop is dead."
		NH Rep. Tom Alciere - (My new Hero)

2.4.1prepatch 8
drivers/md/md.c line 3722

	for (; i < MD_SB_DISKS && str; i++) {
		/*
		if ((device = name_to_kdev_t(str))) {
			md_setup_args.devices[minor][i] = device;
		} else {
			printk ("md: Unknown device name, %s.\n", str);
			return 0;
		}
		if ((str = strchr(str, ',')) != NULL)
			str++;
		*/
		
		char *ndevstr;
		ndevstr = strchr(str, ',');	// Goto ','
		if (ndevstr != NULL)
			*ndevstr++ = 0;		// Zero it for proper string
		
		// DEBUG Print device name
		printk("Checking: '%s'\n", str);	
		
		
		// Convert device name to k_dev_t and assign to md_setup_args.devices
		// DEBUG As test, hardcode device names for /dev/md0.0 and /dev/md0.1
		if (minor == 0 && i == 0)
			md_setup_args.devices[minor][i] = name_to_kdev_t("/dev/sdd5");
		else if (minor == 0 && i == 1)
			md_setup_args.devices[minor][i] = name_to_kdev_t("/dev/sde5");
		else
			md_setup_args.devices[minor][i] = name_to_kdev_t(str);
		
		// DEBUG Print out kdevname of md_setup_args.devices
		printk("\t%s\n", kdevname(md_setup_args.devices[minor][i]));
		// DEBUG Print minor and i (insync?)
		printk("minor=%d, i=%d\n",minor, i);
		
		// name_to_kdev_t() returned 0. Invalid device 
		if (md_setup_args.devices[minor][i] == 0) {
			printk ("md: Unknown device name, %s.\n", str);
			return 0;
		}
		// Jump to next devname in str
		str = ndevstr;
	}

Looks to me like this parsing code unnecessarily and rather clumsily re-invents strtok() and should be rewritten to use that function. It takes two nested loops, along the general lines of:

	/*
	outer loop, parse into space-separted strings
	*/
	for( p = strtok(str, " ") ; p != NULL ; p = strtok(NULL, " ")	{
		/*
		inner loop using comma separator
		*
		for( q = strtok( p, ",") ; q != NULL ; q = strtok(NULL, ",")	{

		}
	}

I suspect that I've misunderstood some constraint here. Perhaps the more complex code you posted is necessary, but I'd like to know why.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

strtok is not reentrant and cannot be nested this way without saving __strtok. strsep would work.

-Andi

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

The original parsing code is this:

		if ((str = strchr(str, ',')) != NULL)
			str++;
Which effectivly steps through
		/dev/sda1,/dev/sdab1,/dev/sdc1
like this
		str == /dev/sda1,/dev/sdab1,/dev/sdc1
		str == /dev/sdab1,/dev/sdc1
		str == /dev/sdc1

My code:	char *ndevstr;
		ndevstr = strchr(str, ',');
		if (ndevstr != NULL)	*ndevstr++ = 0;	
		...
		str = ndevstr

Works perfectly. I don't find it 'clumsy' or more complex at all. (I don't care for strtok, nor did I even know the kernel had it)

However I don't see this critique of coding style helping the problem I'm seeing:

	name_to_kdev_t(str);
Returns a bad value. Yet
	name_to_kdev_t("/dev/sdd5");
does not. The strange thing is
	printk("Checking: '%s'\n", str);

shows str does infact contain a proper string.

It appears str does not get passed to name_to_kdev_t() properly, and I have no idea why. Both my testing code and the original code exhibit the same problem.

"Nobody will ever be safe until the last cop is dead."
		NH Rep. Tom Alciere - (My new Hero)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

But be careful:

strsep() in kernel skips zero length strings, but strsep glibc wouldn't do.

Regards

Ingo Oeser

Note: I implemented it to replace strtok and even did a patch to
replace all occourences of it, but got no response and so stopped working on this issue. If there is still interest, I would do it again.

10.+11.03.2001 - 3. Chemnitzer LinuxTag <http://www.tu-chemnitz.de/linux/tag>
         <<<<<<<<<<<<       come and join the fun       >>>>>>>>>>>>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/