Re: [Qemu-devel] [PATCH 1/3] block/qcow2: refactoring of threaded encryption code

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2019-09-06 at 13:00 -0500, Eric Blake wrote:
> On 9/6/19 12:31 PM, Maxim Levitsky wrote:
> > This commit tries to clarify few function arguments,
> > and add comments describing the encrypt/decrypt interface
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/qcow2-cluster.c |  8 +++----
> >  block/qcow2-threads.c | 53 ++++++++++++++++++++++++++++++++++---------
> >  2 files changed, 46 insertions(+), 15 deletions(-)
> > 
> > +++ b/block/qcow2-threads.c
> > @@ -234,15 +234,19 @@ 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)
> 
> Pre-existing bug in alignment...
No problem will fix.

> 
> > +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset,
> > +                  uint64_t guest_offset, void *buf, size_t len,
> > +                  Qcow2EncDecFunc func)
> 
> ...so this would be a great time to fix it.
> 
> >  {
> >      BDRVQcow2State *s = bs->opaque;
> > +
> > +    uint64_t offset = s->crypt_physical_offset ?
> > +        host_cluster_offset + offset_into_cluster(s, guest_offset) :
> > +        guest_offset;
> > +
> >      Qcow2EncDecData arg = {
> >          .block = s->crypto,
> > -        .offset = s->crypt_physical_offset ?
> > -                      file_cluster_offset + offset_into_cluster(s, offset) :
> > -                      offset,
> > +        .offset = offset,
> 
> I'm ambivalent on whether the new 'offset' variable gains us anything.
> But it doesn't hurt.
I added it, so that I won't need to wrap the lines even more that 
they are wrapped already since I increased the length of
the parameter names a bit.

> 
> 
> >          .buf = buf,
> >          .len = len,
> >          .func = func,
> > @@ -251,18 +255,45 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> >      return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
> >  }
> >  
> > +
> > +/*
> > + * qcow2_co_encrypt()
> > + *
> > + * Encrypts a sector size aligned contiguous area
> > + *
> > + * @host_cluster_offset - on disk offset of the cluster in which
> > + *                        the buffer resides
> > + *
> > + * @guest_offset - guest (virtual) offset of the buffer
> > + * @buf - buffer with the data to encrypt
> > + * @len - length of the buffer
> > + *
> > + * Note that the area is not cluster aligned and might cross a cluster
> > + * boundary
> 
> Umm, how is it possible for a sector to cross a cluster boundary?  All
> clusters are sector-aligned, and encryption only works on aligned
> sectors.  Oh, I see - if @len is a multiple larger than sector size,
> then we have multiple sectors, and then indeed we may cross clusters.
> But then the docs about being 'a sector size aligned contiguous area' is
> not quite right.

Why? the written area is always both aligned on _sector_ boundary
and multiple of the sector size. At least that what I see from
the existing asserts.


> 
> > + *
> > + *
> > + */
> >  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_cluster_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len)
> >  {
> > -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
> >                               qcrypto_block_encrypt);
> 
> 
> Another alignment worth fixing up while in the area.
> 
> >  }
> >  
> > +
> > +/*
> > + * qcow2_co_decrypt()
> > + *
> > + * Decrypts a sector size aligned contiguous area
> > + * Same function as qcow2_co_encrypt
> > + *
> > + */
> > +
> >  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_cluster_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len)
> >  {
> > -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
> >                               qcrypto_block_decrypt);
> 
> and again.
> 
> >  }
> > 
> 
> 


Best regards,
	Thanks for the review,
		Maxim Levitsky