Re: [Qemu-devel] [PATCH v2 03/37] hw/block/fdc: Implement tray status

[Max Reitz <mreitz@redhat.com>]

On 2015-03-04 at 09:00, Kevin Wolf wrote:
> Am 09.02.2015 um 18:11 hat Max Reitz geschrieben:
>> The tray of an FDD is open iff there is no medium inserted (there are
>> only two states for an FDD: "medium inserted" or "no medium inserted").
>>
>> This results in the tray being reported as open if qemu has been started
>> with the default floppy drive, which breaks some tests. Fix them.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   hw/block/fdc.c             | 20 +++++++++++++---
>>   tests/fdc-test.c           |  4 +---
>>   tests/qemu-iotests/067.out | 60 +++++++---------------------------------------
>>   tests/qemu-iotests/071.out |  2 --
>>   tests/qemu-iotests/081.out |  1 -
>>   tests/qemu-iotests/087.out |  6 -----
>>   6 files changed, 26 insertions(+), 67 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 2bf87c9..0c5a6b4 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -192,6 +192,8 @@ typedef struct FDrive {
>>       uint8_t ro;               /* Is read-only           */
>>       uint8_t media_changed;    /* Is media changed       */
>>       uint8_t media_rate;       /* Data rate of medium    */
>> +
>> +    bool media_inserted;      /* Is there a medium in the tray */
>>   } FDrive;
>>   
>>   static void fd_init(FDrive *drv)
>> @@ -261,7 +263,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
>>   #endif
>>           drv->head = head;
>>           if (drv->track != track) {
>> -            if (drv->blk != NULL && blk_is_inserted(drv->blk)) {
>> +            if (drv->media_inserted) {
> I suspect that with the removal of blk_is_inserted() in several places,
> floppy passthrough (host_floppy block driver) is now even more broken
> than before, potentially not noticing removal of a medium any more.
>
> While checking this, I noticed that since commit 21fcf360,
> bdrv_media_changed() is completely unused. Media change has therefore
> probably been broken since at least May 2012.
>
> Considering this, it might actually be reasonable enough to remove the
> block driver. It's definitely better than having it there, but not
> working any better than host_device.
>
> Of course, alternatively you would also be welcome to fix the device
> model and reintroduce bdrv_media_changed() and blk_is_inserted() calls
> where necessary.

Urrggghhh... I guess it's my own fault for touching floppy code.

I guess I'll rethink this patch while keeping in mind that 
bdrv_is_inserted() (as opposed to blk_is_inserted()) actually is 
necessary here because we won't notice if the host medium has been 
exchanged otherwise.

Max