On Thu, May 06, 2004 at 09:45:00AM +0200, Quentin Garnier wrote:
(but didn't add to the gnats entry...)
> Hi all,
> 
> There is a bug in the way readdisklabel() interacts with device label
> generation routines in lower level driver.  It is clearly apparent with
> cd(4), but might as well happen with other devices for which it can be
> completely normal to have neither a disklabel nor a MBR.
> 
> kern/subr_disk_mbr.c is not used by all archs, only by amd64, i386 and
> ibmnws as it seems.  So the issue as I will describe it only happens on
> those archs.  That doesn't mean some other archs don't have their
> readdisklabel() bugged, too.

FWIW I added subr_disk_mbr.c in order to start the process of commoning
up a load of code that should never have got itself MD...

> When cd(4) wants to generate a disklabel, it calls cdgetdisklabel().
> That function first tries to call readdisklabel(), and if that function
> returns an error, it outputs it on the console and use a default label.
> 
> On i386 (I confirm it does about the same on -1-6, too), readdisklabel()
> tries to find a MBR first, then tries to directly read a disklabel on
> the device.
> 
> The difference between -1-6 and -current from after July, 7th, 2003, is
> that the MI readdisklabel() does not return an error if it couldn't read
> neither a MBR not a disklabel.
> 
> That avoids the message "cd0: no disk label" we can see on our -1-6
> hosts, but that makes cdgetdisklabel immediately return, not generating
> a correct label.
> 
> The reason why disklabel shows 4.2BSD for 'a' is that the MI
> readdisklabel sets that by default, in case it doesn't find a MBR or a
> label.

Yes - otherwise everything suffers from serious grief since nothing
excpect to have to use cd0d.

Before I wrote subr_disk_mbr.c all the partitions used to get set to
cover the whole disk....

> I think this is not a correct behaviour, and even though I agree we
> should avoid printing an error message when the case of actually having
> a MBR or a label on a CD-ROM would be very unexpected.
> 
> My proposal is the following:  making readdisklabel() return an empty
> string in case no relevant data was found while no error happened. Then
> the device routine that calls readdisklabel() can check the length of
> the returned message and not print it if it is empty.

That just adds code to all the drivers - which just isn't (and shouldn't be)
necessary.

> Somehow I wonder what happens for devices that first calls their default
> labelling function, and then call readdisklabel(), which inconditionally
> overwrites type for 'a'.  I'd like some input on what to do here, too.

The obvious thing is for the default label (returned by readdisklabel)
to contain the fs_type from the raw partition in entry 'a'.

This patch should suffice.

	David

Index: subr_disk_mbr.c
===================================================================
RCS file: /cvsroot/src/sys/kern/subr_disk_mbr.c,v
retrieving revision 1.4
diff -u -p -r1.4 subr_disk_mbr.c
--- subr_disk_mbr.c	8 Oct 2003 04:25:46 -0000	1.4
+++ subr_disk_mbr.c	6 May 2004 20:32:59 -0000
@@ -225,13 +225,15 @@ readdisklabel(dev, strat, lp, osdep)
 	if (lp->d_partitions[RAW_PART].p_size == 0)
 		lp->d_partitions[RAW_PART].p_size = 0x1fffffff;
 	lp->d_partitions[RAW_PART].p_offset = 0;
+	if (lp->d_partitions[RAW_PART].p_fstype == FS_UNUSED)
+		lp->d_partitions[RAW_PART].p_fstype = FS_BSDFFS;
 
 	/*
 	 * Set partition 'a' to be the whole disk.
 	 * Cleared if we find an mbr or a netbsd label.
 	 */
 	lp->d_partitions[0].p_size = lp->d_partitions[RAW_PART].p_size;
-	lp->d_partitions[0].p_fstype = FS_BSDFFS;
+	lp->d_partitions[0].p_fstype = lp->d_partitions[RAW_PART].p_fstype;
 
 	/* get a buffer and initialize it */
 	a.bp = geteblk((int)lp->d_secsize);

-- 
David Laight: david@l8s.co.uk