Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3)

[Carlo Marcelo Arenas Belon]

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