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

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

[Dave Cinege]

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

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)

[-- Attachment #2: md-test.c --]
[-- Type: text/x-csrc, Size: 1335 bytes --]

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;
	}

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

[Sandy Harris]

Dave Cinege wrote:
> 
> ... '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.)

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.
 
> 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;
>         }
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

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

[Andi Kleen]

On Sat, Jan 20, 2001 at 04:58:56PM -0500, Sandy Harris wrote:
> 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.

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 majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

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

[Ingo Oeser]

On Sat, Jan 20, 2001 at 11:28:51PM +0100, Andi Kleen wrote:
> On Sat, Jan 20, 2001 at 04:58:56PM -0500, Sandy Harris wrote:
> > 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.
> 
> strtok is not reentrant and cannot be nested this way without 
> saving __strtok. strsep would work.

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 majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

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

[Dave Cinege]

Sandy Harris wrote:
> Looks to me like this parsing code unnecessarily and rather clumsily 
> re-invents strtok

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 majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

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

[Dave Cinege]

Douglas Gilbert wrote:
> 
> Dave,
> Look at the dmesg output and check that your
> "Kernel command line:" is what you think it
> is. Some older versions of lilo truncate it.
> Here is mine (which is what I expected):
> 
> Kernel command line: auto BOOT_IMAGE=lin240 ro root=803 scsihosts=imm:advansys:a
> dvansys:aha1542

No that is not the problem. I'm using GRUB (LILO == poopoo) and 
have looked at this throughly.

I can see from the dmesg via my debugging printk's output that str is properly
being passed. 

-- 
"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 majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/