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

[Markus Armbruster]

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.

> 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));
 }
 
 /*

It's also not const-correct.