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

[Kevin Wolf]

Am 18.03.2019 um 18:21 hat Markus Armbruster geschrieben:
> 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?

If the given BlockBackend isn't empty, you could fall back to the node
name, i.e. bdrv_get_device_or_node_name(blk_bs(blk)).

If you want to report an error because it's empty, I think we actually
know that it's -drive because you can't create a BlockBackend with
-blockdev and flash devices don't create empty BlockBackends either. So
using blk_name() there looks fine.

Maybe we want a BlockBackend level function that returns the
BlockBackend name if it isn't empty, and the root node name otherwise?

Kevin