Re: [PATCH 13/20] block: Mark bdrv_co_get_allocated_file_size() and callers GRAPH_RDLOCK

[Kevin Wolf]

Am 01.05.2023 um 21:03 hat Stefan Hajnoczi geschrieben:
> On Tue, Apr 25, 2023 at 07:31:51PM +0200, Kevin Wolf wrote:
> > @@ -5778,6 +5779,7 @@ int64_t coroutine_fn 
> > bdrv_co_get_allocated_file_size(BlockDriverState *bs)
> >  {
> >      BlockDriver *drv = bs->drv;
> >      IO_CODE();
> > +    assert_bdrv_graph_readable();
> 
> Is there a need for runtime assertions in functions already checked by
> TSA?
> 
> I guess not. Otherwise runtime assertions should have been added in many
> of the other functions marked GRAPH_RDLOCK in this series.

I guess we're a bit inconsistent with this. Emanuele started adding the
assertions everywhere before I added the TSA annotations. Since then,
I've tended to leave the assertions from Emanuele's patches (such as
this one) around, but didn't add assertions in new patches.

The point in favour for still having assertions is that people using gcc
won't see TSA failures, but runtime assertions will still work for them.
I don't think we should have them in every GRAPH_RDLOCK function, but
having them in one central place in each call chain (i.e. the block.c
wrappers for BlockDriver callbacks) does make sense to me. So if we
stick to this standard, we'd keep this assertion.

But if you prefer, I can drop it. I assume that enough developers run
with clang that the additional assertion doesn't buy us that much. And
I compile with clang anyway when applying patches.

Kevin

signature.asc
Description: PGP signature