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

[Alex Williamson <alex.williamson@hp.com>]

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.