From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K3aUA-0006Lb-6E for qemu-devel@nongnu.org; Tue, 03 Jun 2008 13:39:50 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K3aU8-0006I1-D9 for qemu-devel@nongnu.org; Tue, 03 Jun 2008 13:39:49 -0400 Received: from [199.232.76.173] (port=41914 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K3aU8-0006Hi-8C for qemu-devel@nongnu.org; Tue, 03 Jun 2008 13:39:48 -0400 Received: from tapir.sajinet.com.pe ([66.139.79.212]:37349) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1K3aU7-0002vD-Th for qemu-devel@nongnu.org; Tue, 03 Jun 2008 13:39:48 -0400 Date: Tue, 3 Jun 2008 13:01:22 -0500 From: Carlo Marcelo Arenas Belon Subject: Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3) Message-ID: <20080603180122.GA21838@tapir> References: <1211865917.8201.56.camel@bling> <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> <1212502910.10496.7.camel@bling> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1212502910.10496.7.camel@bling> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: Alexander Graf , qemu-devel@nongnu.org On Tue, Jun 03, 2008 at 08:21:50AM -0600, Alex Williamson wrote: > > 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. this is a "sorta" valid test for the GET_CONFIGURATION call as what is needed here is the "active" profile, that is dependent on the type of media that is loaded into a multi profile MMC device. using the size of the media as an indication of what type it is, is definitely weak and likely to fail on edge cases (as indicated by the comments) and should be replaced for a better heuristic if there is one that can be used instead. Carlo