On 30.04.19 11:44, Vladimir Sementsov-Ogievskiy wrote: > 30.04.2019 11:38, Vladimir Sementsov-Ogievskiy wrote: >> 29.04.2019 19:37, Max Reitz wrote: >>> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote: >>>> Background: decryption will be done in threads, to take benefit of it, >>>> we should move it out of the lock first. >>> >>> ...which is safe after your commit c972fa123c73501b4, I presume. >>> >>> (At first glance, the patched looked a bit weird to me because it >>> doesn't give a reason why dropping the lock around >>> qcrypto_block_decrypt() would be OK.) >>> >>>> But let's go further: it turns out, that for locking around switch >>>> cases we have only two variants: when we just do memset(0) not >>>> releasing the lock (it is useless) and when we actually can handle the >>>> whole case out of the lock. So, refactor the whole thing to reduce >>>> locked code region and make it clean. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >>>> Reviewed-by: Alberto Garcia <be...@igalia.com> >>>> --- >>>> block/qcow2.c | 46 ++++++++++++++++++++++------------------------ >>>> 1 file changed, 22 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/block/qcow2.c b/block/qcow2.c >>>> index 46e8e39da5..fcf92a7eb6 100644 >>>> --- a/block/qcow2.c >>>> +++ b/block/qcow2.c >>>> @@ -1983,6 +1983,7 @@ static coroutine_fn int >>>> qcow2_co_preadv(BlockDriverState *bs, uint64_t offset, >>>> ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, >>>> &cluster_offset); >>> >>> Isn't this the only function in the loop that actually needs the lock? >>> Wouldn't it make more sense to just take it around this call? >>> >> >> Hmm, looks correct, I'll resend. >> >> > > Or not, actually, we may have several qcow2_get_data_offset calls under one > lock, > if clusters are different kinds of ZERO. So, I think better to keep it as is > for now.
Hm, but how is this relevant? For one thing, if that was a problem if some other party concurrently changes the image, then that would be a problem in general. Keeping the lock would hide it for different kinds of read-as-zero clusters, but it would still appear if data clusters and other clusters are interleaved, wouldn’t it? Also, this is a coroutine. As long as nothing yields, nothing gets concurrent access. I don’t see anything outside of qcow2_get_cluster_offset() that could yield as long as we only see read-as-zero clusters. Max
signature.asc
Description: OpenPGP digital signature
