Re: [Qemu-devel] [PATCH v2 4/4] block: convert qcrypto_block_encrypt|decrypt to take bytes offset

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3558 bytes --]

On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> Instead of sector offset, take the bytes offset when encrypting
> or decrypting data.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/crypto.c         |  8 ++++----
>  block/qcow.c           |  7 +++++--
>  block/qcow2-cluster.c  |  8 +++-----
>  block/qcow2.c          |  4 ++--
>  crypto/block-luks.c    | 12 ++++++++----
>  crypto/block-qcow.c    | 12 ++++++++----
>  crypto/block.c         | 14 ++++++++------
>  crypto/blockpriv.h     |  4 ++--
>  include/crypto/block.h | 14 ++++++++------
>  9 files changed, 48 insertions(+), 35 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 40daa77188..6b8d88efbc 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -426,8 +426,8 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
>              goto cleanup;
>          }
>  
> -        if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
> -                                  cur_bytes, NULL) < 0) {
> +        if (qcrypto_block_decrypt(crypto->block, offset + bytes_done,
> +                                  cipher_data, cur_bytes, NULL) < 0) {

How close are we to getting rid of even needing 'sector_num' as a variable?

> +++ b/block/qcow.c
> @@ -456,7 +456,9 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>                          if (i < n_start || i >= n_end) {
>                              Error *err = NULL;
>                              memset(s->cluster_data, 0x00, 512);
> -                            if (qcrypto_block_encrypt(s->crypto, start_sect + i,
> +                            if (qcrypto_block_encrypt(s->crypto,
> +                                                      start_sect + i *
> +                                                      BDRV_SECTOR_SIZE,

Umm, not the same.  You want (start_sect + i) * BDRV_SECTOR_SIZE.

> +++ b/block/qcow2-cluster.c
> @@ -396,15 +396,13 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
>  {
>      if (bytes && bs->encrypted) {
>          BDRVQcow2State *s = bs->opaque;
> -        int64_t sector = (s->crypt_physical_offset ?
> +        int64_t offset = (s->crypt_physical_offset ?
>                            (cluster_offset + offset_in_cluster) :
> -                          (src_cluster_offset + offset_in_cluster))
> -                         >> BDRV_SECTOR_BITS;
> +                          (src_cluster_offset + offset_in_cluster));
>          assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>          assert((bytes & ~BDRV_SECTOR_MASK) == 0);

Pre-existing, but we could use osdep.h macros here, as in QEMU_IS_ALIGNED().

> +++ b/crypto/block-luks.c
> @@ -1403,29 +1403,33 @@ static void qcrypto_block_luks_cleanup(QCryptoBlock *block)
>  
>  static int
>  qcrypto_block_luks_decrypt(QCryptoBlock *block,
> -                           uint64_t startsector,
> +                           uint64_t offset,
>                             uint8_t *buf,
>                             size_t len,
>                             Error **errp)
>  {
> +    assert(!(offset % QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
> +    assert(!(len % QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));

Again, QEMU_IS_ALIGNED() might be easier to read - but this time, it's
in code you're adding.

Looking forward to v3.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]