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

[Michael Tokarev]

On 29.02.2012 20:07, Paolo Bonzini wrote:
> Il 29/02/2012 17:00, Michael Tokarev ha scritto:
>> 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.
> 
> It's not very different from leaking memory.  It's just the way C works.
> 
> In the future, you may add unlock around image access like in qcow2, and
> then an unlock/lock pair would be confusing without the lock/unlock outside.
> 
> If you are worried about forgetting to unlock, add owner tracking to
> qemu-coroutine-lock.c, similar to PTHREAD_MUTEX_ERRORCHECK.  That would
> be quite useful.

You're replying to the lest significant part of my whole comment.

I'm not worrying about forgetting about unlock or about freeing
memory.  It is just one of the more things that needs remembering,
that's all.

I'm "worrying" about adding irrelevant code into already complex
routine, and adding the same code to two places, too, while it
very naturally fits into one common wrapper with this proposed
infrastructure.

Only when we actually start making these drivers less "lock-dependent"
and let them to be "a bit more" reenterant, -- only then it will
make sense to move the lock/unlock logic _down_ to these routines,
to take locks for as little time as possible.

This is what happened with qcow and qcow2 drivers in the 3/3 patch, --
the change which you missed.  There, the drivers actually gained
_new_ wrapper, without any additional cleanups, which does
dispatching from rw to read or write.  And only for these two
examples your original NACK for this 1/3 part can be applied :)

The "cleanup" you're referring to above is not a cleanup, it
is particular driver development, and I can try to do that
too, just not in this series, it is a completely separate
issue.

Thanks,

/mjt