Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 18.03.2019 um 17:03 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>> 
>> > Am 08.03.2019 um 18:03 hat Markus Armbruster geschrieben:
>> >> >> bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr 
>> >> >> size,
>> >> >>                                  Error **errp)
>> >> >> {
>> >> >>     int64_t blk_len;
>> >> >>     int ret;
>> >> >> 
>> >> >>     blk_len = blk_getlength(blk);
>> >> >>     if (blk_len < 0) {
>> >> >>         error_setg_errno(errp, -blk_len,
>> >> >>                          "can't get size of block backend '%s'",
>> >> >>                          blk_name(blk));
>> >> >>         return false;
>> >> >>     }
>> >> >>     if (blk_len != size) {
>> >> >>         error_setg(errp, "device requires %" PRIu64 " bytes, "
>> >> >>                    "block backend '%s' provides %" PRIu64 " bytes",
>> >> >>                    size, blk_name(blk), blk_len);
>> >> >
>> >> > Should size use HWADDR_PRIu?
>> >> 
>> >> Yes.
>> >> 
>> >> > I'm not sure if printing the BlockBackend name is a good idea because
>> >> > hopefully one day the BlockBackend will be anonymous even for the flash
>> >> > devices.
>> >> 
>> >> Hmm.  Tell me what else I can use to identify the troublemaker to the
>> >> user.
>> >
>> > My hope was that a caller of this would prefix the right context. For
>> > example, if the device were created by -device, the error message would
>> > be prefixed with the whole "-device driver=pflash...:" string, which
>> > gives enough context to the user.
>> >
>> > Machine code that instantiates the device based on -drive should
>> > probably do something similar.
>> 
>> I'm very much in favor of reporting errors like "where to fix it: what's
>> wrong".  Heck, I created infrastructure for it and put it to use.
>> Sadly, we're not even close to being able to using it as intended here.
>> 
>> Ideally, we'd annotate every bit of configuration with its location
>> information, and use that for error messages.
>> 
>> In reality, we make only half-hearted attempts here and there to keep
>> location information around.  It doesn't make it to realize():
>> 
>>     $ qemu-system-ppc64 -S -display none -M sam460ex -drive 
>> if=pflash,format=raw,file=1b.img
>>     qemu-system-ppc64: Initialization of device cfi.pflash01 failed: device 
>> requires 1048576 bytes, block backend 'pflash0' provides 512 bytes
>
> Good enough even without the 'pflash0' block backend name, honestly. If
> I know that QEMU magically creates a block backend named 'pflash0', I
> should also be able to figure out where 'cfi.pflash01' comes from.

    $ qemu-system-ppc64 -S -display none -M taihu -drive 
if=pflash,format=raw,file=eins.img -drive 
if=pflash,unit=1,format=raw,file=zwei.img
    qemu-system-ppc64: Initialization of device cfi.pflash02 failed: device 
requires 2097152 bytes, block backend provides 512 bytes

Which one's short, eins.img or zwei.img?

Good enough anyway?

>> As you can see, the qdev core prefixes the error with "Initialization of
>> device TYPE-NAME failed: " instead a location.  Better than nothing.
>> Ambiguous when there's more than one device of this type.
[...]
>> I hope you'll understand why I'm declining to drain this swamp right
>> now.
>
> Yes, but I'll add the question if now is really the time to optimise
> error messages for -drive.

It isn't, and ...

>> Naming the block backend helps the user and is simple.  You tell me
>> it'll break some day (if I understand you correctly).  Pity.  Any other
>> ideas on how to help the user that don't involve swamp draining?
>
> I thought that the very work you're doing right now on pflash is
> motivated by -blockdev support. The moment you achieve this goal, you'll
> get an empty string as the block backend name here.

... that's exactly why I'm asking for other ideas.

> Of course, a message like "device requires 1048576 bytes, block backend
> '' provides 512 bytes" is not the end of the world either. It's just a
> decision whether our preferred interface with the best error messages is
> -drive or -blockdev.

Given the sad state of location tracking, I'm afraid we do need to map
from BlockBackend to some text that helps the user with finding the
place to correct the problem.

Any ideas on that?