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 <mlevi...@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
