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

[Kevin Wolf]

Am 19.03.2019 um 14:25 hat Markus Armbruster geschrieben:
> >> > 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.

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.

> > 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.

Kevin