On Thu, Apr 27, 2023 at 01:12:43PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 20:37 hat Eric Blake geschrieben:
> > On Tue, Apr 25, 2023 at 07:31:39PM +0200, Kevin Wolf wrote:
> > > There is a bdrv_co_getlength() now, which should be used in coroutine
> > > context.
> > >
> > > Signed-off-by: Kevin Wolf <kw...@redhat.com>
> > > ---
> > >
> > > -static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
> > > - bool want_zero,
> > > - int64_t offset, int64_t
> > > count,
> > > - int64_t *pnum, int64_t
> > > *map,
> > > - BlockDriverState **file)
> > > +static int coroutine_fn GRAPH_RDLOCK
> > > +qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t
> > > offset,
> > > + int64_t count, int64_t *pnum, int64_t *map,
> > > + BlockDriverState **file)
> > > {
> >
> > Should the commit message also call out that you are using this
> > opportunity to add GRAPH_RDLOCK annotations on affected functions?
>
> This is not just some additional change I did on the side, but the patch
> doesn't compile (on clang with TSA enabled) without it because
> bdrv_co_getlength() is GRAPH_RDLOCK, so its callers already must hold
> the lock.
Okay, that makes sense.
>
> I can be more explicit about it in this patch, though I expect that the
> same situation might happen more often in the future, and I'm not sure
> if we want to mention that in the commit message any more than why we're
> passing through an Error pointer.
Still, a commit message something like:
There is a bdrv_co_getlength() now, which should be used in coroutine
context, which in turn requires more accurate function annotations.
would have helped me review faster.
>
> > Overall, all changes in this patch make sense, but I'm reluctant to
> > add R-b unless you confirm whether this was a rebase mistake (where
> > the annotations were intended to be added in a later patch).
>
> No, it's intentional.
All right, you've answered my question.
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
