Re: [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 14:25 hat Markus Armbruster geschrieben:
>> 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.

Anchor [*] for later.

>> >> 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.
>
> Ah, yes, makes sense.
>
>> > (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.
>
> Leaves the question whether blk_name() can ever be called for an
> anonymous empty BlockBackend. If it can, we must explicitly handle that
> case because it would crash with that code.

Outcome of the discussion below: we should not change blk_name().  You can
call the unchanged blk_name() safely for any BlockBackend, but its value
is useful only for named ones.

Leaves it unable to fill my need, so I circle back to [*]: maybe we want
a BlockBackend level function that returns some text that helps the user
with finding the place to correct the problem.

We'd still have to figure out when it could used safely, and when its
value would actually be useful.

I'm afraid I've fallen behind too far on block layer evolution to
actually code and document this function without a lot of handholding.
Perhaps you doing it would be less work for you.

Note that quite a few existing uses of blk_name() are for error messages
and such.  These are actually wrong (and getting wronger with -blockdev
becoming used more widely).  Fixing them would be a nice way to
introduce the function.

>> > 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"?
>
> You can change it to "returns a non-empty string for BlockBackends that
> aren't both anonymous and empty". This doesn't sound like much of a
> simplification to me. If you want to make it "returns a non-empty
> string, period", you need to figure out what to do with anonymous
> empty BlockBackends.
>
> You have to consider a few more things if you want it to return some
> meaningful string instead of just a string. Currently, if it returns a
> non-empty string, you will get the BlockBackend back when you call
> blk_by_name() with this string. If you start returning node names from
> blk_name(), but leave blk_by_name() unchanged, you don't know whether
> you'll get a BlockBackend when you call blk_by_name().
>
> If you do change both functions, in the context of blk_by_name(), a
> BlockBackend won't have a single name any more, but you can identify it
> both by its actual name and the node name of its root node (if present).
>
> I'll stop here, but making this change feels like it could have many
> implications. None of these implications look actually bad, but in order
> to stay consistent, a lot of places need to be changed. I'm not sure if
> it's worth the effort.

You're right, this is a swamp we should not enter.