Re: [Qemu-devel] [PATCH 1/3] Combine bdrv_read and bdrv_write to bdrv_rw

[Michael Tokarev <mjt@tls.msk.ru>]

On 29.02.2012 19:53, Paolo Bonzini wrote:
> Il 29/02/2012 00:54, Michael Tokarev ha scritto:
>> -static coroutine_fn int cow_co_write(BlockDriverState *bs, int64_t sector_num,
>> -                                     const uint8_t *buf, int nb_sectors)
>> +static coroutine_fn int cow_co_rw(BlockDriverState *bs, int64_t sector_num,
>> +                                  uint8_t *buf, int nb_sectors, bool is_write)
>>  {
>>      int ret;
>>      BDRVCowState *s = bs->opaque;
>>      qemu_co_mutex_lock(&s->lock);
>> -    ret = cow_write(bs, sector_num, buf, nb_sectors);
>> +    ret = is_write ? cow_write(bs, sector_num, buf, nb_sectors) :
>> +                     cow_read(bs, sector_num, buf, nb_sectors);
>>      qemu_co_mutex_unlock(&s->lock);
>>      return ret;
> 
> NACK,
> 
> the real cleanup here would be to move the lock/unlock inside cow_read
> and cow_write.

And how it will be a cleanup?

The whole cow code (and a few others) is not reenterant.  Merely
moving this lock/unlock stuff inth actual methods eliminates two
current wrappers in cow_co_write() and cow_co_read(), which are
exactly the same now, and moves this exactly the same code into
actual methods, which has nothing to do with locking - they're
not reenterant, and they deal with internal to the format stuff.
Having this common locking layer on top and _outside_ of the
actual work helps removing irrelevant code from important paths.
Also, it will be too easy to forgot to unlock it there by doing
just "return" somewhere.

So that'll be not a cleanup at all.

Thanks,

/mjt