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