Re: [Qemu-devel] [RFC 06/10] fdc: add physical disk sizes

[John Snow <jsnow@redhat.com>]

On 07/03/2015 09:38 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> 2.88MB capable drives can accept 1.44MB floppies,
>> for instance. To rework the pick_geometry function,
>> we need to know if our current drive can even accept
>> the type of disks we're considering.
>>
>> NB: This allows us to distinguish between all of the
>> "total sectors" collisions between 1.20MB and 1.44MB
>> diskette types.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/block/fdc.c | 27 +++++++++++++++++++++++----
>>  1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 4004b98..6e5f87b 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -59,6 +59,11 @@ typedef enum FDriveRate {
>>      FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
>>  } FDriveRate;
>>  
>> +typedef enum FDriveSize {
>> +    FDRIVE_SIZE_350,
>> +    FDRIVE_SIZE_525
>> +} FDriveSize;
>> +
>>  typedef struct FDFormat {
>>      FDriveType drive;
>>      uint8_t last_sect;
>> @@ -95,15 +100,15 @@ static const FDFormat fd_formats[] = {
>>      { FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, },
>>      /* 1.2 MB 5"1/4 floppy disks */
>>      { FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, },
>> -    { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, },
>> +    { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, }, /* conflicts w/ #0 */
>>      { FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, },
>>      { FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, },
>> -    { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, },
>> +    { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, }, /* conflicts w/ #1 */
>>      /* 720 kB 5"1/4 floppy disks */
>> -    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, },
>> +    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, }, /* conflicts w/ #13 */
>>      { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, },
>>      /* 360 kB 5"1/4 floppy disks */
>> -    { FDRIVE_DRV_120,  9, 40, 1, FDRIVE_RATE_300K, },
>> +    { FDRIVE_DRV_120,  9, 40, 1, FDRIVE_RATE_300K, }, /* conflicts w/ #32 */
>>      { FDRIVE_DRV_120,  9, 40, 0, FDRIVE_RATE_300K, },
>>      { FDRIVE_DRV_120, 10, 41, 1, FDRIVE_RATE_300K, },
>>      { FDRIVE_DRV_120, 10, 42, 1, FDRIVE_RATE_300K, },
> 
> Can you elaborate on what "conflicts w/" means here?
> 

Bad documentation, yes.

The "conflict" here is that the number of sectors described by this
format collides with another. It's not clear by glancing at the table
which ones conflict.

The physical media size can be used to disambiguate these cases.

>> @@ -116,6 +121,20 @@ static const FDFormat fd_formats[] = {
>>      { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
>>  };
>>  
>> +__attribute__((__unused__))
>> +static FDriveSize drive_size(FDriveType drive)
>> +{
>> +    switch (drive) {
>> +    case FDRIVE_DRV_120:
>> +        return FDRIVE_SIZE_525;
>> +    case FDRIVE_DRV_144:
>> +    case FDRIVE_DRV_288:
>> +        return FDRIVE_SIZE_350;
>> +    default:
>> +        return -1;
> 
> Works, but isn't so nice.  When I see a function returning an enum type,
> I expect the return value to be one of its enum constants.
> 
> You can either return int, or add the error value to the enum type.
> 

I think I'll add an error case to the enum. FDRIVE_SIZE_INVALID, etc.

>> +    }
>> +}
>> +
>>  #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
>>  #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))

Thanks!