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

[Cédric Le Goater]

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