Kevin Wolf <kw...@redhat.com> writes:
> 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.
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.
