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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> 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:
[...]
>> >> 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"?