Re: [Qemu-devel] [PATCH v2 3/4] block: convert crypto driver to bdrv_co_preadv|pwritev

[Eric Blake]

On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> Make the crypto driver implement the bdrv_co_preadv|pwritev
> callbacks,  and also use bdrv_co_preadv|pwritev for I/O
> with the protocol driver beneath.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  block/crypto.c | 103 
> +++++++++++++++++++++++++++++----------------------------
>  1 file changed, 53 insertions(+), 50 deletions(-)
> 

>  static coroutine_fn int
> -block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> -                      int remaining_sectors, QEMUIOVector *qiov)
> +block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> +                       QEMUIOVector *qiov, int flags)

>  {
>      BlockCrypto *crypto = bs->opaque;
> -    int cur_nr_sectors; /* number of sectors in current iteration */
> +    uint64_t cur_bytes; /* number of bytes in current iteration */
>      uint64_t bytes_done = 0;
>      uint8_t *cipher_data = NULL;
>      QEMUIOVector hd_qiov;
>      int ret = 0;
> -    size_t payload_offset =
> -        qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
> +    size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);

Pre-existing: is size_t the right type, or can we overflow a 64-bit
offset on a 32-bit host?

> +    uint64_t sector_num = offset / BDRV_SECTOR_SIZE;
> +
> +    assert((offset % BDRV_SECTOR_SIZE) == 0);
> +    assert((bytes % BDRV_SECTOR_SIZE) == 0);

The osdep.h macros might be nicer than open-coding; furthermore, if
desired, you could shorten to:

assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));


>  
>  static coroutine_fn int
> -block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
> -                       int remaining_sectors, QEMUIOVector *qiov)
> +block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t 
> bytes,
> +                        QEMUIOVector *qiov, int flags)
>  {

Hmm - you don't set supported_write_flags.  But presumably, if the
underlying BDS supports BDRV_REQUEST_FUA, then crypto can likewise
support that flag by passing it through to the underlying device after
encryption.

>      BlockCrypto *crypto = bs->opaque;
> -    int cur_nr_sectors; /* number of sectors in current iteration */
> +    uint64_t cur_bytes; /* number of bytes in current iteration */
>      uint64_t bytes_done = 0;
>      uint8_t *cipher_data = NULL;
>      QEMUIOVector hd_qiov;
>      int ret = 0;
> -    size_t payload_offset =
> -        qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
> +    size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
> +    uint64_t sector_num = offset / BDRV_SECTOR_SIZE;
> +
> +    assert((offset % BDRV_SECTOR_SIZE) == 0);
> +    assert((bytes % BDRV_SECTOR_SIZE) == 0);

Same comment as for read.

> @@ -611,8 +613,9 @@ BlockDriver bdrv_crypto_luks = {
>      .bdrv_truncate      = block_crypto_truncate,
>      .create_opts        = &block_crypto_create_opts_luks,
>  
> -    .bdrv_co_readv      = block_crypto_co_readv,
> -    .bdrv_co_writev     = block_crypto_co_writev,
> +    .bdrv_refresh_limits = block_crypto_refresh_limits,
> +    .bdrv_co_preadv      = block_crypto_co_preadv,
> +    .bdrv_co_pwritev     = block_crypto_co_pwritev,
>      .bdrv_getlength     = block_crypto_getlength,
>      .bdrv_get_info      = block_crypto_get_info_luks,
>      .bdrv_get_specific_info = block_crypto_get_specific_info_luks,

Looks weird when = isn't consistently aligned, but we use more than one
space.  My preference is to just use one space everywhere, as adding a
longer name to the list doesn't require a mass re-format of all other
entries; but I'm not opposed when people like the aligned = for
legibility.  So up to you if you do anything in response to my nit.

Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature