[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device |
Date: |
Fri, 05 Apr 2013 17:11:24 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Stefan Hajnoczi <address@hidden> writes:
> On Fri, Apr 5, 2013 at 2:57 PM, Markus Armbruster <address@hidden> wrote:
>> Stefan Hajnoczi <address@hidden> writes:
>>
>>> On Fri, Apr 05, 2013 at 12:56:07PM +0200, Markus Armbruster wrote:
>>>> Stefan Hajnoczi <address@hidden> writes:
>>>>
>>>> > What is the highest addressable sector on an empty CD-ROM? Nothing is
>>>> > addressable so produce an error.
>>>> >
>>>> > This patch prevents a divide-by-zero in ide_set_sector() since
>>>> > s->sectors and s->heads would be 0. Not to mention that a sector=-1
>>>> > argument would be nonsense.
>>>> >
>>>> > Note that WIN_READ_NATIVE_MAX can be triggered using hdparm -N 1024
>>>> > /dev/cdrom. The LBA bit will be set to 1 though, so the only easy way
>>>> > to go down the ide_set_sector() CHS code path which divides by zero is
>>>> > to comment out the s->select & 0x40 case for testing.
>>>>
>>>> Suggests you did that.
>>>>
>>>> Have you tried the reproducer with a physical drive? Does it fail the
>>>> command when empty, too?
>>>
>>> Believe it or not, I don't have access to an ATAPI CD-ROM drive. Would
>>> you be able to try out hdparm -N 1024 /dev/cdrom?
>>>
>>> Note that READ NATIVE MAX is optional, real drives may not implement it
>>> since it seems geared towards the Host Protected Area feature which
>>> makes no sense on CD-ROMs. (The idea is a reserved area on the disk
>>> where system data can be stored and the OS will not touch it.)
>>>
>>> Stefan
>>
>> # hdparm -N /dev/cdrom
>>
>> /dev/cdrom:
>> READ_NATIVE_MAX_ADDRESS failed: Input/output error
>> # hdparm -N 1024 /dev/cdrom
>>
>> /dev/cdrom:
>> setting max visible sectors to 1024 (temporary)
>> READ_NATIVE_MAX_ADDRESS failed: Input/output error
>> READ_NATIVE_MAX_ADDRESS failed: Input/output error
>>
>> Same with and without media.
>>
>> If the command makes no sense for CD-ROMs, and generally isn't
>> implemented by them, we should consider not implementing either, by
>> clearing its IDE_CD bit in ide_cmd_table.
>
> Thank you!
>
> You're getting the same result that we get in the guest. This looks good.
>
> There's no harm in supporting READ NATIVE MAX when the CD-ROM is
> inserted. It's basically another way of finding out the block device
> size.
>
> I also like this fix better than blacklisting the command since it now
> protects us in case HD or CFA devices ever have nb_sectors = 0 too.
> In other words, it's more defensive than just fixing CD-ROMs.
Good point.
I'd probably do both, simply because a command that isn't available is
100% bug-free. But that's in the realm of artistic license.
Reviewed-by: Markus Armbruster <address@hidden>
Re: [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device, Stefan Hajnoczi, 2013/04/10