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

[Daniel P. Berrange]

On Sat, Sep 16, 2017 at 06:54:58PM +0200, Max Reitz wrote:
> 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 <address@hidden>
> > ---
> >  block/crypto.c | 104 
> > +++++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 56 insertions(+), 48 deletions(-)
> 
> Reviewed-by: Max Reitz <address@hidden>
> 
> 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.

In the end i'm going with this approach - just dropping the previous
patch entirely, since 99% of what it does is then removed in this
patch and the next one.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|