and a bit about empty lines
13.09.2019 20:27, Maxim Levitsky wrote:
> Change the qcow2_co_encrypt to just receive full host and
> guest offsets and in pariticular remove the
> offset_in_cluster parameter of do_perform_cow_encrypt,
> since it is misleading, because that offset can be larger than
> cluster size currently.
>
> Remove the do_perform_cow_encrypt by merging it with
> qcow2_co_encrypt
>
> Also document the qcow2_co_encrypt arguments to prevent
> that bug from happening again
>
> Signed-off-by: Maxim Levitsky <mlevi...@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
[..]
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..b31d45fb2b 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -234,15 +234,15 @@ static int qcow2_encdec_pool_func(void *opaque)
> }
>
> static int coroutine_fn
> -qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> - uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc
> func)
> +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_offset,
> + uint64_t guest_offset, void *buf, size_t len,
> + Qcow2EncDecFunc func)
> {
> BDRVQcow2State *s = bs->opaque;
> +
Don't see real benefit in separating one variable initialization from another
one here,
but I don't mind, it's OK of course.
> Qcow2EncDecData arg = {
> .block = s->crypto,
> - .offset = s->crypt_physical_offset ?
> - file_cluster_offset + offset_into_cluster(s, offset) :
> - offset,
> + .offset = s->crypt_physical_offset ? host_offset : guest_offset,
> .buf = buf,
> .len = len,
> .func = func,
> @@ -251,18 +251,73 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t
> file_cluster_offset,
> return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
> }
>
> +
Hmm, in this file doubled empty lines
are used to separate "Compression" and "Cryptography"
sections. And all other splits are one-empty-line.
> +/*
> + * qcow2_co_encrypt()
> + *
> + * Encrypts one or more contiguous aligned sectors
> + *
> + * @host_offset - underlying storage offset of the first sector of the
> + * data to be encrypted
> + *
> + * @guest_offset - guest (virtual) offset of the first sector of the
> + * data to be encrypted
> + *
> + * @buf - buffer with the data to encrypt, that after encryption
> + * will be written to the underlying storage device at
> + * @host_offset
> + *
> + * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
> + *
> + * Depending on the encryption method, @host_offset and/or @guest_offset
> + * may be used for generating the initialization vector for
> + * encryption.
> + *
> + * Note that while the whole range must be aligned on sectors, it
> + * does not have to be aligned on clusters and can also cross cluster
> + * boundaries
> + */
> int coroutine_fn
> -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> - uint64_t offset, void *buf, size_t len)
> +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
> + uint64_t guest_offset, void *buf, size_t len)
> {
> - return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> - qcrypto_block_encrypt);
> +
This empty line is not needed for sure
> + BDRVQcow2State *s = bs->opaque;
But it's common practice to split variables definition from code by empty line
here
> + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
> + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
> + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE));
> + assert(s->crypto);
> +
> + if (!len) {
> + return 0;
> + }
> +
> + return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> + qcrypto_block_encrypt);
> }
>
> +
IMHO extra one (see above)
> +/*
> + * qcow2_co_decrypt()
> + *
> + * Decrypts one or more contiguous aligned sectors
> + * Similar to qcow2_co_encrypt
> + */
> +
Here you replaced one extra empty line by another
> int coroutine_fn
> -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> - uint64_t offset, void *buf, size_t len)
> +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
> + uint64_t guest_offset, void *buf, size_t len)
> {
> - return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> - qcrypto_block_decrypt);
--
Best regards,
Vladimir
