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

[Kevin Wolf]

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.

> 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.

Yes, but I'll add the question if now is really the time to optimise
error messages for -drive.

> 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.

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.

Kevin