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, > > return (EACCES); > > case SD_CMD_FAILURE: > > ASSERT(mutex_owned(SD_MUTEX(un))); > > + /* > > + * A multisession audio cd can have an unreadable > > + * fdisk sector, but there could be readable data > > + * in a separate session. Accept this and let > > + * the code build a default disk label later on. > > + */ > > + if (ISCD(un)) > > + break; > > return (ENOMEM); > > } > > > > > > > > > > Comments? > > > > > > This message posted from opensolaris.org > > _______________________________________________ > > ufs-discuss mailing list > > ufs-discuss at opensolaris.org > > > > > > Juergen Keil jk at tools.de Tools GmbH +49 (228) 9858011
