Re: [Qemu-devel] [PATCH v3 5/7] block: convert crypto driver to bdrv_co_preadv|pwritev

[Max Reitz <mreitz@redhat.com>]

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

On 2017-09-12 13:28, 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 <berrange@redhat.com>
> ---
>  block/crypto.c | 104 +++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 56 insertions(+), 48 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Some notes below.

> diff --git a/block/crypto.c b/block/crypto.c
> index 49d6d4c058..d004e9cef4 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c

[...]

> @@ -410,37 +414,33 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
>          goto cleanup;
>      }
>  
> -    while (remaining_sectors) {
> -        cur_nr_sectors = remaining_sectors;
> +    while (bytes) {
> +        cur_bytes = bytes;
>  
> -        if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {
> -            cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;
> +        if (cur_bytes > (BLOCK_CRYPTO_MAX_SECTORS * sector_size)) {
> +            cur_bytes = BLOCK_CRYPTO_MAX_SECTORS * sector_size;

It's a bit weird that now the bounce buffer's size is now no longer
fixed at 1 MB but variable depending on the crypto driver's block size.
It also doesn't seem too intentional when looking at the first patch's
commit message...

In any case, that would be an issue in the previous patch, though.  In
general, I'm wondering whether maybe you should squash this patch into
the previous one...  Yes, that would make the for a larger patch, but it
wouldn't leave some not-quite-right state in between where sector_size
is generally assumed to be equal to BDRV_SECTOR_SIZE -- which it is in
practice, but not necessarily in theory.

>          }

Also, just a note: It would be shorter with a MIN(). :-)

(cur_bytes = MIN(bytes, BLOCK_CRYPTO_MAX_SECTORS * sector_size))

Max


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