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

[Markus Armbruster]

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

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.

Say you decide to do the right thing *now*, i.e. get configuration
location information to realize().  What location information?  The
device being realized is a mandatory onboard device, created by code.
There is no configuration connected to it.

Then you think hard about the code, and realize that the -drive
if=pflash kind of configures this device (and its backend) anyway.  But
only "kind of", because the configuration is optional; the device is
created even when the user doesn't specify -drive if=pflash.  And then
there's still no configuration connected to it.

Even if a backend exists, it needn't be user-configured.  It could also
be created by default.  Again, we have nowhere to point to.

Even if we proclaim there won't ever be errors involving onboard devices
without backends or with default backends (haha), connecting the device
to the right piece of configuration is still tricky.  You're in a twisty
little maze of special cases, all different.

Now, if our machines were built entirely from configuration rather than
code, the ugly "no location" cases wouldn't exist.  With systematic
configuration location tracking, we could then do something like

    qemu-system-ppc64: /usr/share/qemu/ppc64/sam460ex.cfg:42: device requires 
1048576 bytes, block backend provides 512 bytes
    qemu-system-ppc64: -drive if=pflash,format=raw,file=1b: info: this is the 
block backend

where /usr/share/qemu/ppc64/sam460ex.cfg:42 points to the spot that
configures the onboard cfi.pflash01.

For a device created with -device, we'd get

    qemu-system-ppc64: -device cfi.pflash01,drive=pfl0: device requires 1048576 
bytes, block backend provides 512 bytes
    qemu-system-ppc64: -drive if=none,id=pfl0,format=raw,file=1b: info: this is 
the block backend

(hypothetical; cfi.pflash01 is not available with -device)

I hope you'll understand why I'm declining to drain this swamp right
now.

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?