I am volunteering to be sponsor of this case.
Larry
Juergen Keil wrote:
OK, I've filed CR 6505720 wrong default disk label on x86, for an audio cd,
and need a sponsor for the fix.
My suggested fix is at http://www.tools.de/solaris/opensolaris/sd_label
Contributor agreement # : OS0003
Larry, would you sponsor the fix for me?
The changes look good to me. I can help to integrated if needed.
Larry
From: J??rgen Keil jk at tools.de
To: ufs-discuss at opensolaris.org
Cc:
Subject: [ufs-discuss] Re: sd(7d) uses broken default disk labels on
x86, for an audio cd
Date: Fri, 15 Dec 2006 11:54:12 +0100
I wrote:
First issue, where sd(7d) uses a bogus disk label for (multisession /
mixed audio+data)
cds on x86.
I think there are two issues here:
1. when sd_read_fdisk() cannot read sector 0, it returns an
SD_CMD_FAILURE error, but doesn't set un-un_solaris_offset
and un-un_solaris_size.
And it doesn't clear the VTOC info.
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/common/io/scsi/targets/
sd.c#4915
4915 bufp = kmem_zalloc(blocksize, KM_SLEEP);
4916 rval = sd_send_scsi_READ(un, bufp, blocksize, 0, path_flag);
4919 if (rval != 0) {
4920 SD_ERROR(SD_LOG_ATTACH_DETACH, un,
4921 sd_read_fdisk: fdisk read err\n);
4922 kmem_free(bufp, blocksize);
4923 return (SD_CMD_FAILURE);
4924 }
instead of returning immediatelly, shouldn't this jump to the done
label,
just like the case when sector 0 is readable but doesn't contain a valid
MBR signature?
4979 /*
4980 * Endian-independent signature check
4981 */
4982 if (((sigbuf[1] 0xFF) != ((MBB_MAGIC 8) 0xFF)) ||
4983 (sigbuf[0] != (MBB_MAGIC 0xFF))) {
4984 SD_ERROR(SD_LOG_ATTACH_DETACH, un,
4985 sd_read_fdisk: no fdisk\n);
4986 bzero(un-un_fmap, sizeof (struct fmap) * FD_NUMPART);
4987 rval = SD_CMD_SUCCESS;
4988 goto done;
4989 }
2. The second issue is that no default label is constructed by
sd_validate_geometry()
when sd_read_fdisk() returns with SD_CMD_FAILURE, which happens when
sector 0
wasn't readable. The code returns with an ENOMEM error.
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/common/io/scsi/targets/
sd.c#4478
4478 /*
4479 * Note: This will set up un-un_solaris_size and
4480 * un-un_solaris_offset.
4481 */
4482 switch (sd_read_fdisk(un, capacity, lbasize,
path_flag)) {
4483 case SD_CMD_RESERVATION_CONFLICT:
4484 ASSERT(mutex_owned(SD_MUTEX(un)));
4485 return (EACCES);
4486 case SD_CMD_FAILURE:
4487 ASSERT(mutex_owned(SD_MUTEX(un)));
4488 return (ENOMEM);
4489 }
On SPARC, sd_read_fdisk() cannot fail, so we keep going and a default
label is constructed further down, at line 4595:
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/common/io/scsi/targets/
sd.c#4595
4595 /*
4596 * If a valid label was not found, AND if no reservation
conflict
4597 * was detected, then go ahead and create a default label
(4069506).
4598 */
4599 if (un-un_f_default_vtoc_supported (label_error !=
EACCES)) {
4600 if (un-un_f_geometry_is_valid == FALSE) {
4601 sd_build_default_label(un);
4602 }
4603 label_error = 0;
4604 }
I think the x86 platform should behave just like sparc in this case:
although
the MBR sector 0 wasn't readable, we do have obtained valid blocksize and
capacity from the optical drive, and we can use that capacity to
construct a
default label.
I'm currently using the following fix:
diff -r c1119b5a9ffa usr/src/uts/common/io/scsi/targets/sd.c
--- a/usr/src/uts/common/io/scsi/targets/sd.c Wed Nov 29 16:09:14
2006 -0800
+++ b/usr/src/uts/common/io/scsi/targets/sd.c Sun Dec 10 21:03:32
2006 +0100
@@ -4916,8 +4958,9 @@ sd_read_fdisk(struct sd_lun *un, uint_t
if (rval != 0) {
SD_ERROR(SD_LOG_ATTACH_DETACH, un,
sd_read_fdisk: fdisk read err\n);
- kmem_free(bufp, blocksize);
- return (SD_CMD_FAILURE);
+ bzero(un-un_fmap, sizeof (struct fmap) * FD_NUMPART);
+ rval = SD_CMD_FAILURE;
+ goto done;
}
mbp = (struct mboot *)bufp;
diff -r c1119b5a9ffa usr/src/uts/common/io/scsi/targets/sd.c
--- a/usr/src/uts/common/io/scsi/targets/sd.c Wed Nov 29 16:09:14
2006 -0800
+++ b/usr/src/uts/common/io/scsi/targets/sd.c Sun Dec 10 23:15:26
2006 +0100
@@ -4482,6 +4524,14 @@ sd_validate_geometry(struct sd_lun *un,
