Re: [PATCH] m25p80: Warn the user when the backend file is too small for the device

["Cédric Le Goater" <clg@kaod.org>]

On 11/15/22 15:34, Peter Maydell wrote:
> On Tue, 15 Nov 2022 at 14:22, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Currently, when a block backend is attached to a m25p80 device and the
>> associated file size does not match the flash model, QEMU complains
>> with the error message "failed to read the initial flash content".
>> This is confusing for the user.
> 
> The commit message says we get an unhelpful error if the
> file size "does not match"...
> 
>> Improve the reported error with a new message regarding the file size.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/block/m25p80.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 02adc87527..e0515e2a1e 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -1606,6 +1606,14 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
>>       if (s->blk) {
>>           uint64_t perm = BLK_PERM_CONSISTENT_READ |
>>                           (blk_supports_write_perm(s->blk) ? BLK_PERM_WRITE : 0);
>> +
>> +        if (blk_getlength(s->blk) < s->size) {
> 
> ...but the code change is only checking for "too small".
> 
> What happens if the user provides a backing file that is too large ?

That's ok because the blk_pread() call following, which loads in RAM
the initial data, won't fail.

It might be better to enforce a strict check on the size to avoid
further confusion ? and change the error message to be clear.
  
> 
>> +            error_setg(errp,
>> +                       "backend file is too small for flash device %s (%d MB)",
>> +                       object_class_get_name(OBJECT_CLASS(mc)), s->size >> 20);
> 
> This potentially reports to the user a size which isn't the
> right one for them to use to set the size of the backing file,
> if that required size isn't an exact number of MB.

True. We have a few devices which size is below 1MB. Using a KB unit
should be fine.

Thanks,

C.

> 
>> +            return;
>> +        }
>> +
>>           ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
>>           if (ret < 0) {
>>               return;
>> --
>> 2.38.1
> 
> thanks
> -- PMM