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

[Kevin Wolf]

Am 19.03.2019 um 11:34 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > 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.
> 
> Now I'm confused.  You just convinced me I need more than blk_name().
> Hmm, testing...
> 
>     $ qemu-system-x86_64 -nodefaults -S -display none -blockdev 
> node-name=pflash0,driver=file,filename=eio.img -machine pflash0=pflash0
>     qemu-system-x86_64: Initialization of device cfi.pflash01 failed: can't 
> read block backend '': Input/output error
> 
> Yes, I need more.

This is not an empty BlockBackend, it has a root node inserted. You do
need more for non-empty BlockBackends.

> > Maybe we want a BlockBackend level function that returns the
> > BlockBackend name if it isn't empty, and the root node name otherwise?
> 
> Makes sense to me.
> 
> What about calling it blk_name()?  ;-P
> 
> This quick voodoo patch crashes:
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index edad02a0f2..3c2527f9c0 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -642,7 +642,7 @@ void monitor_remove_blk(BlockBackend *blk)
>   */
>  const char *blk_name(const BlockBackend *blk)
>  {
> -    return blk->name ?: "";
> +    return blk->name ?: bdrv_get_device_or_node_name(blk_bs(blk));
>  }

I don't know the backtrace of your crash, but I assume it is because
blk_name() is called for an empty BlockBackend, so blk_bs(blk) == NULL.
(By the way, it could be just bdrv_get_node_name() in this context,
because we already know that the device name doesn't exist, but that one
doesn't like NULL either.)

Either this assumption is wrong, or my analysis that flash devices never
have empty BlockBackends attached was wrong, or this is a call from a
different place and a new function called only from flash instead of
changing blk_name() for all callers might actually have worked.

Kevin