From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1K3XOu-0006PX-1D
	for qemu-devel@nongnu.org; Tue, 03 Jun 2008 10:22:12 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1K3XOs-0006O9-Aw
	for qemu-devel@nongnu.org; Tue, 03 Jun 2008 10:22:11 -0400
Received: from [199.232.76.173] (port=33651 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1K3XOs-0006O2-79
	for qemu-devel@nongnu.org; Tue, 03 Jun 2008 10:22:10 -0400
Received: from g5t0006.atlanta.hp.com ([15.192.0.43]:1802)
	by monty-python.gnu.org with esmtps
	(TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60)
	(envelope-from <alex.williamson@hp.com>) id 1K3XOs-0003gn-JZ
	for qemu-devel@nongnu.org; Tue, 03 Jun 2008 10:22:10 -0400
Subject: Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure
	command (v3)
From: Alex Williamson <alex.williamson@hp.com>
In-Reply-To: <2EA004FA-68CF-4989-BD54-42E3AA231C35@suse.de>
References: <1211865917.8201.56.camel@bling>
	<B34520FE-89D9-4D93-A0D6-48C4DED102EA@suse.de>
	<1212004084.6821.30.camel@lappy>
	<13EAABC4-ED0C-4B91-9DF4-9486F0A1259E@suse.de>
	<1212418735.6656.32.camel@lappy>
	<8F70DBBA-DB05-4B03-886B-A2182F3A3AC6@suse.de>
	<1212444720.13411.12.camel@lappy> <1212446737.13411.15.camel@lappy>
	<2EA004FA-68CF-4989-BD54-42E3AA231C35@suse.de>
Content-Type: text/plain
Date: Tue, 03 Jun 2008 08:21:50 -0600
Message-Id: <1212502910.10496.7.camel@bling>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Reply-To: qemu-devel@nongnu.org
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Alexander Graf <agraf@suse.de>
Cc: qemu-devel@nongnu.org

Hi Alex,

On Tue, 2008-06-03 at 15:48 +0200, Alexander Graf wrote:

> On Jun 3, 2008, at 12:12 AM, Alex Williamson wrote:

> > @@ -1741,16 +1845,11 @@
> >             /* 
> >              * the number of sectors from the media tells us which
> > profile
> >              * to use as current.  0 means there is no media
> > -             *
> > -             * XXX: fails to detect correctly DVDs with less data
> > burned
> > -             *      than what a CD can hold
> >              */
> > -            if (s -> nb_sectors) {
> > -                if (s -> nb_sectors > CD_MAX_SECTORS)
> > -                    cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
> > -                else
> > -                    cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM);
> > -            }
> > +            if (media_is_dvd(s))
> > +                cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
> > +            else if (media_is_cd(s))
> > +                cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM);
> 
> 
> After having looked at the spec and a real dvd rom output I got
> uncertain if this is correct. Shouldn't capabilities provide the
> capabilities of the drive and not the medium?

Yes, I agree, this is a pretty weak test, however, that's a topic for a
different patch.  I'm not changing the existing logic here with this
patch.

> Apart from that the patch looks fine to me.

Great, thanks for the review!  If there are no other comments, could
someone please apply this to the tree?  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.