Am 18.03.2019 um 18:21 hat Markus Armbruster geschrieben:
> Kevin Wolf <kw...@redhat.com> writes:
>
> > Am 18.03.2019 um 17:03 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kw...@redhat.com> 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
