phantom empty cd

phantom empty cd

[Mike Brudevold]

With regards to a problem with phantom empty cd's visible from
nautilus [1], I have a question about the following code (from
cdrom_id.c scsi_cmd_run):

if ((cmd->sg_io.info & SG_INFO_OK_MASK) != SG_INFO_OK) {
    errno = EIO;
    ret = -1;
    if (cmd->sg_io.masked_status & CHECK_CONDITION) {
        ret = ERRCODE(cmd->_sense.u);
        if (ret = 0)
            ret = -1;
    }
}

I do not have a copy of the spec, so I can only guess, but if
CHECK_CONDITION is set, the function can only return error (-1) if
cmd->_sense.u is zero (ERRCODE can never be negative).  I experience
the phantom empty cd problem, and when I return -1 from this function
rather than checking for CHECK_CONDITION, the phantom CD problem goes
away, as cdrom_id errors out trying to read the TOC and simply prints
the results.

Alternatively to the above potential solution, I notice that my cd-rw
drive will report that profile 0x02 is the current profile when there
is no cd in the drive.  If you query all the profiles, it will return
0x0a, 0x09, 0x08, and 0x02, but none of these profiles will have their
current bit set.  Would it be advisable to verify that the profile
reported as current in the header is actually current according to the
profile descriptor?  Seems to me that these two should be in sync.

Mike

[1] https://bugs.launchpad.net/ubuntu/+source/udev/+bug/562978

Re: phantom empty cd

[Martin Pitt]

[-- Attachment #1.1: Type: text/plain, Size: 2323 bytes --]

Hello Mike,

Mike Brudevold [2010-04-14 20:54 -0500]:
> if ((cmd->sg_io.info & SG_INFO_OK_MASK) != SG_INFO_OK) {
>     errno = EIO;
>     ret = -1;
>     if (cmd->sg_io.masked_status & CHECK_CONDITION) {
>         ret = ERRCODE(cmd->_sense.u);
>         if (ret == 0)
>             ret = -1;
>     }
> }
> 
> I do not have a copy of the spec, so I can only guess, but if
> CHECK_CONDITION is set, the function can only return error (-1) if
> cmd->_sense.u is zero (ERRCODE can never be negative).

This doesn't even depend on the spec, it's quite obvious from the
logic in cdrom_id itself. Good catch!

I'm not sure whether this will help to fix the problem for everyone
(see below), but it does look like a valid and obvious bug to me. I
have a potential patch for this attached, can you test it please? Kay,
does it make sense to you?

I also uploaded this to my personal package archive, so if you are on
Ubuntu you can test it easily (should be built within the next hour):

  https://.launchpad.net/~pitti/+archive/ppa

> I experience the phantom empty cd problem, and when I return -1 from
> this function rather than checking for CHECK_CONDITION, the phantom
> CD problem goes away, as cdrom_id errors out trying to read the TOC
> and simply prints the results.

For the record, this is https://launchpad.net/bugs/562978; the recent
fixes to properly zero out data helped for most people, but not all;
some people still get output like

main: probing: '/dev/sr1'
cd_media_compat: CDROM_DRIVE_STATUS != CDS_DISC_OK
cd_inquiry: INQUIRY: [LITE-ON ][CD-RW SOHR-5238S][4S06]
cd_profiles: GET CONFIGURATION: number of profiles 184
cd_profiles: profile 0x0a
cd_profiles: current profile 0x02
cd_media_toc: READ TOC: len: 2
cd_media_info: disk type 00
ID_CDROM=1
ID_CDROM_CD_R=1
ID_CDROM_CD_RW=1
ID_CDROM_MRW=1
ID_CDROM_MRW_W=1
ID_CDROM_MEDIA=1
ID_CDROM_MEDIA_STATE=blank

The root problem here is that while the CDROM_DRIVE_STATUS ioctl
already said that there's no CD, we second-guess that in
cd_profiles(), and set "cd_media = 1;" if we have a non-zero current
profile. This seems a bit brittle to me?

Thanks,

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[-- Attachment #1.2: 0001-cdrom_id-Do-not-ignore-errors-from-scsi_cmd_run.patch --]
[-- Type: text/x-diff, Size: 3122 bytes --]

From 0b8bf46653a22b5befe4af0b035134260bdc24be Mon Sep 17 00:00:00 2001
From: Martin Pitt <martin.pitt@ubuntu.com>
Date: Thu, 15 Apr 2010 08:56:48 +0200
Subject: [PATCH] cdrom_id: Do not ignore errors from scsi_cmd_run()

scsi_cmd_run() can return positive error messages if we have CHECK_CONDITION
set and get the error code from the SCSI command result. So check the result
for non-zero, not for being negative.

This should fix another cause for "phantom" media in empty CD-ROM drives.

Thanks to Mike Brudevold <mike@brudevold.com> for spotting this!

https://launchpad.net/bugs/562978
---
 extras/cdrom_id/cdrom_id.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/extras/cdrom_id/cdrom_id.c b/extras/cdrom_id/cdrom_id.c
index afb481d..5119013 100644
--- a/extras/cdrom_id/cdrom_id.c
+++ b/extras/cdrom_id/cdrom_id.c
@@ -242,7 +242,7 @@ static int cd_inquiry(struct udev *udev, int fd) {
 	scsi_cmd_set(udev, &sc, 4, 36);
 	scsi_cmd_set(udev, &sc, 5, 0);
 	err = scsi_cmd_run(udev, &sc, fd, inq, 36);
-	if ((err < 0)) {
+	if ((err != 0)) {
 		info_scsi_cmd_err(udev, "INQUIRY", err);
 		return -1;
 	}
@@ -272,7 +272,7 @@ static int cd_profiles(struct udev *udev, int fd)
 	scsi_cmd_set(udev, &sc, 8, sizeof(header));
 	scsi_cmd_set(udev, &sc, 9, 0);
 	err = scsi_cmd_run(udev, &sc, fd, header, sizeof(header));
-	if ((err < 0)) {
+	if ((err != 0)) {
 		info_scsi_cmd_err(udev, "GET CONFIGURATION", err);
 		return -1;
 	}
@@ -292,7 +292,7 @@ static int cd_profiles(struct udev *udev, int fd)
 	scsi_cmd_set(udev, &sc, 8, len);
 	scsi_cmd_set(udev, &sc, 9, 0);
 	err = scsi_cmd_run(udev, &sc, fd, profiles, len);
-	if ((err < 0)) {
+	if ((err != 0)) {
 		info_scsi_cmd_err(udev, "GET CONFIGURATION", err);
 		return -1;
 	}
@@ -448,7 +448,7 @@ static int cd_media_info(struct udev *udev, int fd)
 	scsi_cmd_set(udev, &sc, 8, sizeof(header));
 	scsi_cmd_set(udev, &sc, 9, 0);
 	err = scsi_cmd_run(udev, &sc, fd, header, sizeof(header));
-	if ((err < 0)) {
+	if ((err != 0)) {
 		info_scsi_cmd_err(udev, "READ DISC INFORMATION", err);
 		return -1;
 	};
@@ -482,7 +482,7 @@ static int cd_media_toc(struct udev *udev, int fd)
 	scsi_cmd_set(udev, &sc, 8, sizeof(header));
 	scsi_cmd_set(udev, &sc, 9, 0);
 	err = scsi_cmd_run(udev, &sc, fd, header, sizeof(header));
-	if ((err < 0)) {
+	if ((err != 0)) {
 		info_scsi_cmd_err(udev, "READ TOC", err);
 		return -1;
 	}
@@ -505,7 +505,7 @@ static int cd_media_toc(struct udev *udev, int fd)
 	scsi_cmd_set(udev, &sc, 8, len);
 	scsi_cmd_set(udev, &sc, 9, 0);
 	err = scsi_cmd_run(udev, &sc, fd, toc, len);
-	if ((err < 0)) {
+	if ((err != 0)) {
 		info_scsi_cmd_err(udev, "READ TOC (tracks)", err);
 		return -1;
 	}
@@ -532,7 +532,7 @@ static int cd_media_toc(struct udev *udev, int fd)
 	scsi_cmd_set(udev, &sc, 8, 12);
 	scsi_cmd_set(udev, &sc, 9, 0);
 	err = scsi_cmd_run(udev, &sc, fd, header, sizeof(header));
-	if ((err < 0)) {
+	if ((err != 0)) {
 		info_scsi_cmd_err(udev, "READ TOC (multi session)", err);
 		return -1;
 	}
-- 
1.7.0.4


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: phantom empty cd

[Kay Sievers]

On Thu, Apr 15, 2010 at 09:10, Martin Pitt <martin.pitt@ubuntu.com> wrote:
> I'm not sure whether this will help to fix the problem for everyone
> (see below), but it does look like a valid and obvious bug to me. I
> have a potential patch for this attached, can you test it please? Kay,
> does it make sense to you?

Sure, looks good. Just put it in.

Thanks,
Kay

Re: phantom empty cd

[Robby Workman]

On Thu, 15 Apr 2010 20:35:18 +0200
Kay Sievers <kay.sievers@vrfy.org> wrote:

> On Thu, Apr 15, 2010 at 09:10, Martin Pitt <martin.pitt@ubuntu.com>
> wrote:
> > I'm not sure whether this will help to fix the problem for everyone
> > (see below), but it does look like a valid and obvious bug to me. I
> > have a potential patch for this attached, can you test it please?
> > Kay, does it make sense to you?
> 
> Sure, looks good. Just put it in.


That's what she said.

</bad_humor>  ;-)

-RW