Re: [Qemu-devel] [PATCH 05/27] block/parallels: add get_block_status

[Denis V. Lunev]

On 22/04/15 15:39, Stefan Hajnoczi wrote:
> On Wed, Mar 11, 2015 at 01:27:59PM +0300, Denis V. Lunev wrote:
> > +static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
> > +        int64_t sector_num, int nb_sectors, int *pnum)
> > +{
> > +    BDRVParallelsState *s = bs->opaque;
> > +    int64_t offset;
> > +
> > +    qemu_co_mutex_lock(&s->lock);
> > +    offset = seek_to_sector(s, sector_num);
> > +    qemu_co_mutex_unlock(&s->lock);
> The lock isn't necessary here yet.  It may become necessary when write
> support is added, but probably not even then since seek_to_sector()
> cannot yield (it's not a coroutine function), so there are no possible
> races with other coroutines.
>
> The same also applies for parallels_co_read().  The lock there isn't
> necessary since the block driver is read-only and has no mutable state -
> there is no shared state that needs lock protection.
do you propose to drop the lock here and add it later with write
support (patch 08)? I'd prefer to stay on the safe side. Yes, the
lock is not mandatory at the moment but in 2 patches it will be
mandatory.

I can extend the comment to clarify this.