Kevin Wolf <kw...@redhat.com> writes:
> Am 19.03.2019 um 11:34 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kw...@redhat.com> writes:
>>
>> > Am 18.03.2019 um 18:21 hat Markus Armbruster geschrieben:
[...]
>> >> 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.
It's infinite recursion, actually.
[Many stackframes snipped...]
Note blk=0x555556a7abb0
#232830 0x0000555555d7d396 in blk_name (blk=0x555556a7abb0) at
/work/armbru/qemu/block/block-backend.c:645
#232831 0x0000555555d7c480 in blk_root_get_name (child=0x55555690f140) at
/work/armbru/qemu/block/block-backend.c:148
#232832 0x0000555555d1df59 in bdrv_get_parent_name (bs=0x555556a75690) at
/work/armbru/qemu/block.c:4825
#232833 0x0000555555d1dfcd in bdrv_get_device_or_node_name
(bs=0x555556a75690) at /work/armbru/qemu/block.c:4847
Same blk=0x555556a7abb0
#232834 0x0000555555d7d396 in blk_name (blk=0x555556a7abb0) at
/work/armbru/qemu/block/block-backend.c:645
#232835 0x0000555555aa5c61 in blk_check_size_and_read_all
(blk=0x555556a7abb0, buf=0x7fffcde00000, size=131072, errp=0x7fffffffd760) at
/work/armbru/qemu/hw/block/block.c:42
#232836 0x0000555555aae61e in pflash_cfi01_realize (dev=0x555556a65f00,
errp=0x7fffffffd760) at /work/armbru/qemu/hw/block/pflash_cfi01.c:760
#232837 0x0000555555acf97d in device_set_realized (obj=0x555556a65f00,
value=true, errp=0x7fffffffd920) at /work/armbru/qemu/hw/core/qdev.c:834
#232838 0x0000555555d10324 in property_set_bool (obj=0x555556a65f00,
v=0x555556bb7c60, name=0x555555fdc849 "realized", opaque=0x555556a66450,
errp=0x7fffffffd920) at /work/armbru/qemu/qom/object.c:2074
#232839 0x0000555555d0e59a in object_property_set (obj=0x555556a65f00,
v=0x555556bb7c60, name=0x555555fdc849 "realized", errp=0x7fffffffd920) at
/work/armbru/qemu/qom/object.c:1266
#232840 0x0000555555d1166c in object_property_set_qobject
(obj=0x555556a65f00, value=0x555556bb7c40, name=0x555555fdc849 "realized",
errp=0x7fffffffd920) at /work/armbru/qemu/qom/qom-qobject.c:27
#232841 0x0000555555d0e87f in object_property_set_bool (obj=0x555556a65f00,
value=true, name=0x555555fdc849 "realized", errp=0x7fffffffd920) at
/work/armbru/qemu/qom/object.c:1332
#232842 0x0000555555ace64f in qdev_init_nofail (dev=0x555556a65f00) at
/work/armbru/qemu/hw/core/qdev.c:321
#232843 0x000055555596b5d0 in pc_system_flash_map (pcms=0x555556a38260,
rom_memory=0x555556913f20) at /work/armbru/qemu/hw/i386/pc_sysfw.c:192
#232844 0x000055555596bb1e in pc_system_firmware_init (pcms=0x555556a38260,
rom_memory=0x555556913f20) at /work/armbru/qemu/hw/i386/pc_sysfw.c:322
#232845 0x0000555555962876 in pc_memory_init (pcms=0x555556a38260,
system_memory=0x5555569e1300, rom_memory=0x555556913f20,
ram_memory=0x7fffffffdb28) at /work/armbru/qemu/hw/i386/pc.c:1812
#232846 0x0000555555966176 in pc_init1 (machine=0x555556a38260,
host_type=0x555555f9e52c "i440FX-pcihost", pci_type=0x555555f9e525 "i440FX") at
/work/armbru/qemu/hw/i386/pc_piix.c:181
#232847 0x0000555555966cee in pc_init_v4_0 (machine=0x555556a38260) at
/work/armbru/qemu/hw/i386/pc_piix.c:438
#232848 0x0000555555ad83b1 in machine_run_board_init
(machine=0x555556a38260) at /work/armbru/qemu/hw/core/machine.c:1030
#232849 0x0000555555a328d6 in main (argc=9, argv=0x7fffffffdf58,
envp=0x7fffffffdfa8) at /work/armbru/qemu/vl.c:4463
> (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.)
If I do that, no crash, and the error message looks decent.
> 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.
I think the remaining (and non-trivial) question is what blk_name() is
supposed to do.
Back when I created it, BlockBackend had a somewhat different role, and
blk_name() always returned a non-null, non-empty string.
(Except for a wart: the name becomes empty on HMP drive_del, but that
doesn't really matter here).
blk_name() changed from "you can rely on it to give you a name" to
"maybe it gives you a name, maybe not" in commit e5e785500bf "blockdev:
Separate BB name management".
Should we change it back to "can rely on it"?
