RE: PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct

RE: PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct

[Stuart_Hayes]

At risk of sounding stupid, how can a user space program check the type of
media (DVD vs. CD) that's in the drive?  I think that's what magicdev is
trying to do.
Thanks
Stuart


-----Original Message-----
From: Jens Axboe [mailto:axboe@suse.de]
Sent: Tuesday, February 10, 2004 11:12 AM
To: Hayes, Stuart
Cc: linux-kernel@vger.kernel.org
Subject: Re: PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct


On Tue, Feb 10 2004, Stuart Hayes wrote:
> The attached patch will make the "dvd_read_struct" function in cdrom.c 
> check that the DVD drive can currently do the DVD read structure command 
> before sending the command to the drive.  It does this by checking the 
> "dvd read" feature using the "get configuration" command.
> 
> Currently, cdrom.c only checks that the drive is a DVD drive before 
> allowing the dvd read structure command to go to the drive--it does not 
> make sure that the DVD drive has a DVD in it.  Without this patch, if CD 
> medium is in a DVD drive, and the DVD_READ_STRUCT ioctl is used, the drive
> will spew an ugly "illegal request" error (magicdev does this).

I'm rather anxious about applying anything like this, in my experience
it's much much safer to simply let the command fail. I don't see
anything technically wrong with your approach, I'd just like it tested
on 100 different dvd drives :)

magicdev should be checking the media type itself first.

-- 
Jens Axboe

Re: PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct

[Matt Domsch]

On Tue, Feb 10, 2004 at 12:06:03PM -0600, Stuart_Hayes@Dell.com wrote:
> 
> At risk of sounding stupid, how can a user space program check the type of
> media (DVD vs. CD) that's in the drive?  I think that's what magicdev is
> trying to do.
> Thanks
> Stuart


Stuart, FYI, more linux-kernel ettiquite.

People much prefer to see a quoting style as above (date, from line,
inline >-delimited note you're quoting, followed by non->-delimited
text.  (Such as this).  Outlook doesn't do this very well, as it
always wants to put your response at the top of the message, but with
cut-n-paste you can make the messages look more like what l-k expects.

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct

[Adam Kropelin]

On Tue, Feb 10, 2004 at 12:27:15PM -0600, Matt Domsch wrote:
> On Tue, Feb 10, 2004 at 12:06:03PM -0600, Stuart_Hayes@Dell.com wrote:
> > 
> > At risk of sounding stupid, how can a user space program check the type of
> > media (DVD vs. CD) that's in the drive?  I think that's what magicdev is
> > trying to do.
> > Thanks
> > Stuart
> 
> 
> Stuart, FYI, more linux-kernel ettiquite.
> 
> People much prefer to see a quoting style as above (date, from line,
> inline >-delimited note you're quoting, followed by non->-delimited
> text.  (Such as this).  Outlook doesn't do this very well, as it
> always wants to put your response at the top of the message, but with
> cut-n-paste you can make the messages look more like what l-k expects.

I find OE-QuoteFix and Outlook-QuoteFix do a nice job of making
Outlook Express and Outlook, respectively, more standards compliant in
this regard. In addition to fixing up the message quoting syntax to what
Matt described, they also cleanly handle line wraps. In short, they make
life much easier for those of us tied to Windows boxes at the office.

OE-QuoteFix:
http://home.in.tum.de/~jain/software/oe-quotefix/

Outlook-QuoteFix:
http://flash.to/outlook-quotefix/

--Adam

Re: PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct

[Jens Axboe]

On Tue, Feb 10 2004, Adam Kropelin wrote:
> On Tue, Feb 10, 2004 at 12:27:15PM -0600, Matt Domsch wrote:
> > On Tue, Feb 10, 2004 at 12:06:03PM -0600, Stuart_Hayes@Dell.com wrote:
> > > 
> > > At risk of sounding stupid, how can a user space program check the type of
> > > media (DVD vs. CD) that's in the drive?  I think that's what magicdev is
> > > trying to do.
> > > Thanks
> > > Stuart
> > 
> > 
> > Stuart, FYI, more linux-kernel ettiquite.
> > 
> > People much prefer to see a quoting style as above (date, from line,
> > inline >-delimited note you're quoting, followed by non->-delimited
> > text.  (Such as this).  Outlook doesn't do this very well, as it
> > always wants to put your response at the top of the message, but with
> > cut-n-paste you can make the messages look more like what l-k expects.
> 
> I find OE-QuoteFix and Outlook-QuoteFix do a nice job of making
> Outlook Express and Outlook, respectively, more standards compliant in
> this regard. In addition to fixing up the message quoting syntax to what
> Matt described, they also cleanly handle line wraps. In short, they make
> life much easier for those of us tied to Windows boxes at the office.
> 
> OE-QuoteFix:
> http://home.in.tum.de/~jain/software/oe-quotefix/
> 
> Outlook-QuoteFix:
> http://flash.to/outlook-quotefix/

Maybe it would be a good idea to have this info added to the FAQ, for
those unfortunately individuals that cannot easily switch to a decent
mua?

-- 
Jens Axboe

[FAQ-PATCH] OE-QuoteFix (Was: Re: PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct)

[Adam Kropelin]

On Wed, Feb 11, 2004 at 01:10:50PM +0100, Jens Axboe wrote:
> On Tue, Feb 10 2004, Adam Kropelin wrote:
> > On Tue, Feb 10, 2004 at 12:27:15PM -0600, Matt Domsch wrote:
> > > On Tue, Feb 10, 2004 at 12:06:03PM -0600, Stuart_Hayes@Dell.com wrote:
> > > > 
> > > > At risk of sounding stupid, how can a user space program check the type of
> > > > media (DVD vs. CD) that's in the drive?  I think that's what magicdev is
> > > > trying to do.
> > > > Thanks
> > > > Stuart
> > > 
> > > 
> > > Stuart, FYI, more linux-kernel ettiquite.
> > > 
> > > People much prefer to see a quoting style as above (date, from line,
> > > inline >-delimited note you're quoting, followed by non->-delimited
> > > text.  (Such as this).  Outlook doesn't do this very well, as it
> > > always wants to put your response at the top of the message, but with
> > > cut-n-paste you can make the messages look more like what l-k expects.
> > 
> > I find OE-QuoteFix and Outlook-QuoteFix do a nice job of making
> > Outlook Express and Outlook, respectively, more standards compliant in
> > this regard. In addition to fixing up the message quoting syntax to what
> > Matt described, they also cleanly handle line wraps. In short, they make
> > life much easier for those of us tied to Windows boxes at the office.
> > 
> > OE-QuoteFix:
> > http://home.in.tum.de/~jain/software/oe-quotefix/
> > 
> > Outlook-QuoteFix:
> > http://flash.to/outlook-quotefix/
> 
> Maybe it would be a good idea to have this info added to the FAQ, for
> those unfortunately individuals that cannot easily switch to a decent
> mua?

Here's a patch against the current version of the lkml FAQ.

Richard, I added myself as a contributor to keep consistent style, but
I have no problem if you just want to paste the text in somewhere
appropriate instead. Also feel free to rephrase as needed.

--Adam


--- index.html	Wed Feb 11 18:25:27 2004
+++ index.html.new	Wed Feb 11 18:25:11 2004
@@ -300,6 +300,10 @@
 <A HREF="mailto:andrebalsa@altern.org">Andrew D. Balsa</A>
 </LI>
 
+<LI><TT>ADK:</TT>
+<A HREF="mailto:akropel1@rochester.rr.com">Adam D. Kropelin</A>
+</LI>
+
 <LI><TT>CP :</TT>
 <A HREF="mailto:colin@nyx.net">Colin Plumb</A>
 </LI>
@@ -2911,6 +2915,18 @@
 </LI>
 
 <LI>
+<FONT COLOR="#0000FF">(ADK)</FONT>
+Outlook and Outlook Express are particularly broken with regard to
+proper reply quoting style.
+<A HREF="http://flash.to/outlook-quotefix/">Outlook-QuoteFix</A>
+and <A HREF="http://home.in.tum.de/~jain/software/oe-quotefix/">
+OE-QuoteFix</A> are a couple of useful tools that make
+corresponding on linux-kernel easier for people forced to use
+these damaged mailers. In addition to fixing up the message
+quoting syntax, they also cleanly handle line wraps.
+</LI>
+
+<LI>
 <FONT COLOR="#0000FF">(REG)</FONT>
 Please don't use tabs or multiple spaces to quote text. Use the
 "<TT>&gt; </TT>" sequence instead. Using whitespace to quote text

PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct

[Stuart Hayes]

The attached patch will make the "dvd_read_struct" function in cdrom.c 
check that the DVD drive can currently do the DVD read structure command 
before sending the command to the drive.  It does this by checking the 
"dvd read" feature using the "get configuration" command.

Currently, cdrom.c only checks that the drive is a DVD drive before 
allowing the dvd read structure command to go to the drive--it does not 
make sure that the DVD drive has a DVD in it.  Without this patch, if CD 
medium is in a DVD drive, and the DVD_READ_STRUCT ioctl is used, the drive
will spew an ugly "illegal request" error (magicdev does this).

Thanks
Stuart



diff -BurN linux-2.6.3-rc1/drivers/cdrom/cdrom.c linux-2.6.3-rc1-cdrompatch/drivers/cdrom/cdrom.c
--- linux-2.6.3-rc1/drivers/cdrom/cdrom.c	2004-02-09 15:55:18.000000000 -0600
+++ linux-2.6.3-rc1-cdrompatch/drivers/cdrom/cdrom.c	2004-02-10 09:20:31.000000000 -0600
@@ -1643,8 +1643,41 @@
 	return ret;
 }
 
+static int check_feature_current(struct cdrom_device_info *cdi, __u16 feature)
+{
+	int ret;
+	u_char buf[12];
+	struct cdrom_generic_command cgc;
+	struct cdrom_device_ops *cdo = cdi->ops;
+
+	init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_READ);
+	cgc.cmd[0] = GPCMD_GET_CONFIGURATION;
+	cgc.cmd[1] = 2; /* only get one feature descriptor */
+	cgc.cmd[2] = feature >> 8;
+	cgc.cmd[3] = feature & 0xff;
+	cgc.cmd[8] = 12;
+
+	if ((ret = cdo->generic_packet(cdi, &cgc)))
+		return ret;
+	if (buf[3] < 8) /* i.e., drive doesn't support the feature */
+		return -EMEDIUMTYPE;
+	if ((buf[10] & 1)==0) /* feature is not current */ 
+		return -EMEDIUMTYPE;
+	return 0;
+}
+
 static int dvd_read_struct(struct cdrom_device_info *cdi, dvd_struct *s)
 {
+	int ret;
+
+	/* do a GET_CONFIGURATION to make sure drive can do
+	 * DVD_READ_STRUCTURE command with current media
+	 */
+	ret = check_feature_current(cdi, CDF_DVD_READ);
+	if (ret)
+		return ret;
+
+	/* actually read the requested DVD structure */
 	switch (s->type) {
 	case DVD_STRUCT_PHYSICAL:
 		return dvd_read_physical(cdi, s);
diff -BurN linux-2.6.3-rc1/include/linux/cdrom.h linux-2.6.3-rc1-cdrompatch/include/linux/cdrom.h
--- linux-2.6.3-rc1/include/linux/cdrom.h	2004-02-09 15:55:25.000000000 -0600
+++ linux-2.6.3-rc1-cdrompatch/include/linux/cdrom.h	2004-02-10 09:20:29.000000000 -0600
@@ -722,6 +722,7 @@
 /*
  * feature profile
  */
+#define CDF_DVD_READ	0x1F
 #define CDF_MRW		0x28
 
 /*

Re: PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct

[Jens Axboe]

On Tue, Feb 10 2004, Stuart Hayes wrote:
> The attached patch will make the "dvd_read_struct" function in cdrom.c 
> check that the DVD drive can currently do the DVD read structure command 
> before sending the command to the drive.  It does this by checking the 
> "dvd read" feature using the "get configuration" command.
> 
> Currently, cdrom.c only checks that the drive is a DVD drive before 
> allowing the dvd read structure command to go to the drive--it does not 
> make sure that the DVD drive has a DVD in it.  Without this patch, if CD 
> medium is in a DVD drive, and the DVD_READ_STRUCT ioctl is used, the drive
> will spew an ugly "illegal request" error (magicdev does this).

I'm rather anxious about applying anything like this, in my experience
it's much much safer to simply let the command fail. I don't see
anything technically wrong with your approach, I'd just like it tested
on 100 different dvd drives :)

magicdev should be checking the media type itself first.

-- 
Jens Axboe

Re: PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct

[Jamie Lokier]

Jens Axboe wrote:
> On Tue, Feb 10 2004, Stuart Hayes wrote:
> > The attached patch will make the "dvd_read_struct" function in cdrom.c 
> > check that the DVD drive can currently do the DVD read structure command 
> > before sending the command to the drive.  It does this by checking the 
> > "dvd read" feature using the "get configuration" command.
> > 
> > Currently, cdrom.c only checks that the drive is a DVD drive before 
> > allowing the dvd read structure command to go to the drive--it does not 
> > make sure that the DVD drive has a DVD in it.  Without this patch, if CD 
> > medium is in a DVD drive, and the DVD_READ_STRUCT ioctl is used, the drive
> > will spew an ugly "illegal request" error (magicdev does this).
> 
> I'm rather anxious about applying anything like this, in my experience
> it's much much safer to simply let the command fail. I don't see
> anything technically wrong with your approach, I'd just like it tested
> on 100 different dvd drives :)

Meaning that you don't trust the DVD media test to return true on all
drives when and only when there's DVD media currently in there?

If that's your logic, it follows that userspace programs shouldn't
trust the DVD media test either - they should always call
DVD_READ_STRUCT if they would like to treat DVDs differently to CDs.

So, can you add a flag to the request meaning "if this
DVD_READ_STRUCT request fails with illegal request, don't log it"?

-- Jamie

Re: PATCH (for 2.6.3-rc1) for cdrom driver dvd_read_struct

[Jens Axboe]

On Wed, Feb 11 2004, Jamie Lokier wrote:
> Jens Axboe wrote:
> > On Tue, Feb 10 2004, Stuart Hayes wrote:
> > > The attached patch will make the "dvd_read_struct" function in cdrom.c 
> > > check that the DVD drive can currently do the DVD read structure command 
> > > before sending the command to the drive.  It does this by checking the 
> > > "dvd read" feature using the "get configuration" command.
> > > 
> > > Currently, cdrom.c only checks that the drive is a DVD drive before 
> > > allowing the dvd read structure command to go to the drive--it does not 
> > > make sure that the DVD drive has a DVD in it.  Without this patch, if CD 
> > > medium is in a DVD drive, and the DVD_READ_STRUCT ioctl is used, the drive
> > > will spew an ugly "illegal request" error (magicdev does this).
> > 
> > I'm rather anxious about applying anything like this, in my experience
> > it's much much safer to simply let the command fail. I don't see
> > anything technically wrong with your approach, I'd just like it tested
> > on 100 different dvd drives :)
> 
> Meaning that you don't trust the DVD media test to return true on all
> drives when and only when there's DVD media currently in there?
> 
> If that's your logic, it follows that userspace programs shouldn't
> trust the DVD media test either - they should always call
> DVD_READ_STRUCT if they would like to treat DVDs differently to CDs.
> 
> So, can you add a flag to the request meaning "if this
> DVD_READ_STRUCT request fails with illegal request, don't log it"?

There's the generic cgc->quiet bit for this very purpose. However,
thinking about this particular issue some more,  the feature check
really ought to work for all cases. So I think it's worth a shot
applying the patch as-is. I don't see any better solutions.

-- 
Jens Axboe