[PATCH] Fix cdrom profile enumeration.

[PATCH] Fix cdrom profile enumeration.

[mike]

From: Mike Brudevold <mike@brudevold.com>

---
 extras/cdrom_id/cdrom_id.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/extras/cdrom_id/cdrom_id.c b/extras/cdrom_id/cdrom_id.c
index b6797cd..28b1bfe 100644
--- a/extras/cdrom_id/cdrom_id.c
+++ b/extras/cdrom_id/cdrom_id.c
@@ -261,6 +261,7 @@ static int cd_profiles(struct udev *udev, int fd)
 	struct scsi_cmd sc;
 	unsigned char header[8];
 	unsigned char profiles[512];
+	unsigned int profiles_end;
 	unsigned int cur_profile;
 	unsigned int len;
 	unsigned int i;
@@ -298,7 +299,8 @@ static int cd_profiles(struct udev *udev, int fd)
 	}
 
 	/* device profiles */
-	for (i = 12; i < profiles[11]; i += 4) {
+	profiles_end = 12 + profiles[11];
+	for (i = 12; i < profiles_end; i += 4) {
 		unsigned int profile = (profiles[i] << 8 | profiles[i + 1]);
 		if (profile = 0)
 			continue;
-- 
1.7.0.4

[PATCH] Fix cdrom profile enumeration.

[mike]

From: Mike Brudevold <mike@brudevold.com>

---
 extras/cdrom_id/cdrom_id.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/extras/cdrom_id/cdrom_id.c b/extras/cdrom_id/cdrom_id.c
index b6797cd..28b1bfe 100644
--- a/extras/cdrom_id/cdrom_id.c
+++ b/extras/cdrom_id/cdrom_id.c
@@ -261,6 +261,7 @@ static int cd_profiles(struct udev *udev, int fd)
 	struct scsi_cmd sc;
 	unsigned char header[8];
 	unsigned char profiles[512];
+	unsigned int profiles_end;
 	unsigned int cur_profile;
 	unsigned int len;
 	unsigned int i;
@@ -298,7 +299,8 @@ static int cd_profiles(struct udev *udev, int fd)
 	}
 
 	/* device profiles */
-	for (i = 12; i < profiles[11]; i += 4) {
+	profiles_end = 12 + profiles[11];
+	for (i = 12; i < profiles_end; i += 4) {
 		unsigned int profile = (profiles[i] << 8 | profiles[i + 1]);
 		if (profile = 0)
 			continue;
-- 
1.7.0.4

Re: [PATCH] Fix cdrom profile enumeration.

[Martin Pitt]

Hello Mike,

mike@brudevold.com [2010-04-14 20:38 -0500]:
> -	for (i = 12; i < profiles[11]; i += 4) {
> +	profiles_end = 12 + profiles[11];
> +	for (i = 12; i < profiles_end; i += 4) {

(This could just be written as "i < 12 + profiles[11]" for simplicity)

Do you have some pointers which document this structure, in particular
whether it's really the count and not the last offset? I searched for
some minutes, but didn't find anything. It would be nice to add a
reference to the spec to the code.

Thanks,

Martin

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

Re: [PATCH] Fix cdrom profile enumeration.

[Mike Brudevold]

On Thu, Apr 15, 2010 at 1:43 AM, Martin Pitt <martin.pitt@ubuntu.com> wrote:
> Hello Mike,
>
> mike@brudevold.com [2010-04-14 20:38 -0500]:
>> -     for (i = 12; i < profiles[11]; i += 4) {
>> +     profiles_end = 12 + profiles[11];
>> +     for (i = 12; i < profiles_end; i += 4) {
>
> (This could just be written as "i < 12 + profiles[11]" for simplicity)

I started with that, but GCC was giving me a warning, so I changed it
to what is currently in the patch.  The profiles array is an array of
bytes, so we just needed to either store or cast it as something
larger.

>
> Do you have some pointers which document this structure, in particular
> whether it's really the count and not the last offset? I searched for
> some minutes, but didn't find anything. It would be nice to add a
> reference to the spec to the code.
>

The document I referenced was [1].  Section 10.6.2.1 states that the
"Additional Length field shall be set to ((number of Profile
Descriptors) * 4)."  The size of the feature header is 8 bytes, and we
assume that feature 0 (the profile list) directly follows this.  This
adds 4 bytes, which is how you get the 12 byte start.  You can see
that the size of each profile descriptor is 4 bytes, so it follows
that this is an offset from 12.

In my testing, the git version would not enumerate all available
profiles, whereas this patch would.  I think this went unnoticed
because there is a second set of code (cd_capability_compat) that
checks for what types of media the drive supports, so the information
was still available.

Mike

[1] http://www.t10.org/ftp/t10/document.97/97-263r0.pdf

Re: [PATCH] Fix cdrom profile enumeration.

[Kay Sievers]

On Thu, Apr 15, 2010 at 15:42, Mike Brudevold <mike@brudevold.com> wrote:

> In my testing, the git version would not enumerate all available
> profiles, whereas this patch would.  I think this went unnoticed
> because there is a second set of code (cd_capability_compat) that
> checks for what types of media the drive supports, so the information
> was still available.

We retrieve a list of features, and only feature 0 is the list of
profiles. I reworked the logic to use two loops to iterate over the
lists, and use the variable length field of the "profiles" feature to
find the correct number of profiles. Hope that fixes the problem you
are seeing.

Thanks,
Kay

Re: [PATCH] Fix cdrom profile enumeration.

[Mike Brudevold]

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

On Thu, Apr 15, 2010 at 1:15 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> We retrieve a list of features, and only feature 0 is the list of
> profiles. I reworked the logic to use two loops to iterate over the
> lists, and use the variable length field of the "profiles" feature to
> find the correct number of profiles. Hope that fixes the problem you
> are seeing.

Looks good and much cleaner.  One thing though, the feature_profiles
function is missing profiles 0x8, 0x9, and 0xA.  I have attached a
patch that fixes that.  Otherwise the variable "cd_cd_rom" never gets
set!

Mike

[-- Attachment #2: 0001-cdrom_id-add-missing-profiles-to-feature_profiles.patch --]
[-- Type: text/x-diff, Size: 1099 bytes --]

From 2acfa2a68a1cac0c3d9aaed419e3035b2ac1e8d1 Mon Sep 17 00:00:00 2001
From: Mike Brudevold <mike@brudevold.com>
Date: Thu, 15 Apr 2010 19:55:50 -0500
Subject: [PATCH] cdrom_id: add missing profiles to feature_profiles


Signed-off-by: Mike Brudevold <mike@brudevold.com>
---
 extras/cdrom_id/cdrom_id.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/extras/cdrom_id/cdrom_id.c b/extras/cdrom_id/cdrom_id.c
index 722b8f8..da2785e 100644
--- a/extras/cdrom_id/cdrom_id.c
+++ b/extras/cdrom_id/cdrom_id.c
@@ -274,6 +274,18 @@ static int feature_profiles(struct udev *udev, const unsigned char *profiles, si
 			info(udev, "profile 0x%02x mo\n", profile);
 			cd_mo = 1;
 			break;
+		case 0x08:
+			info(udev, "profile 0x%02x cd_rom\n", profile);
+			cd_cd_rom = 1;
+			break;
+		case 0x09:
+			info(udev, "profile 0x%02x cd_r\n", profile);
+			cd_cd_r = 1;
+			break;
+		case 0x0A:
+			info(udev, "profile 0x%02x cd_rw\n", profile);
+			cd_cd_rw = 1;
+			break;
 		case 0x10:
 			info(udev, "profile 0x%02x dvd_rom\n", profile);
 			cd_dvd_rom = 1;
-- 
1.7.0.4

Re: [PATCH] Fix cdrom profile enumeration.

[Kay Sievers]

On Fri, Apr 16, 2010 at 03:03, Mike Brudevold <mike@brudevold.com> wrote:
> On Thu, Apr 15, 2010 at 1:15 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> We retrieve a list of features, and only feature 0 is the list of
>> profiles. I reworked the logic to use two loops to iterate over the
>> lists, and use the variable length field of the "profiles" feature to
>> find the correct number of profiles. Hope that fixes the problem you
>> are seeing.
>
> Looks good and much cleaner.  One thing though, the feature_profiles
> function is missing profiles 0x8, 0x9, and 0xA.  I have attached a
> patch that fixes that.  Otherwise the variable "cd_cd_rom" never gets
> set!

I guess this dead variable was never really interesting, because there
seem to be no drives which can not read original old CDROMs, if they
can read one of the newer formats. The other, more interesting stuff
we got directly from the kernel ioctl. The debug output now looks
nicer though. :)

Applied. Thanks,
Kay

Re: [PATCH] Fix cdrom profile enumeration.

[Mike Brudevold]

On Tue, Apr 13, 2010 at 10:07 PM,  <mike@brudevold.com> wrote:
>

Ignore this... it was bouncing around the internet for a few days
waiting to be delivered.  Now it appears it has, but not after I
resent it from elsewhere :)