Re: [Qemu-devel] [PATCH v2 2/4] block/qcow2: refactor qcow2_co_preadv_part

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2966 bytes --]

On 30.07.19 16:18, Vladimir Sementsov-Ogievskiy wrote:
> Further patch will run partial requests of iterations of
> qcow2_co_preadv in parallel for performance reasons. To prepare for
> this, separate part which may be parallelized into separate function
> (qcow2_co_preadv_task).
> 
> While being here, also separate encrypted clusters reading to own
> function, like it is done for compressed reading.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json |   2 +-
>  block/qcow2.c        | 206 +++++++++++++++++++++++--------------------
>  2 files changed, 112 insertions(+), 96 deletions(-)

Looks good to me overall, just wondering about some details, as always.

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 93ab7edcea..7fa71968b2 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1967,17 +1967,115 @@ out:
>      return ret;
>  }
>  
> +static coroutine_fn int
> +qcow2_co_preadv_encrypted(BlockDriverState *bs,
> +                           uint64_t file_cluster_offset,
> +                           uint64_t offset,
> +                           uint64_t bytes,
> +                           QEMUIOVector *qiov,
> +                           uint64_t qiov_offset)
> +{
> +    int ret;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint8_t *buf;
> +
> +    assert(bs->encrypted && s->crypto);
> +    assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
> +
> +    /*
> +     * For encrypted images, read everything into a temporary
> +     * contiguous buffer on which the AES functions can work.
> +     * Note, that we can implement enctyption, working on qiov,

-, and s/enctyption/encryption/

> +     * but we must not do decryption in guest buffers for security
> +     * reasons.

"for security reasons" is a bit handwave-y, no?

[...]

> +static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
> +                                             QCow2ClusterType cluster_type,
> +                                             uint64_t file_cluster_offset,
> +                                             uint64_t offset, uint64_t bytes,
> +                                             QEMUIOVector *qiov,
> +                                             size_t qiov_offset)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    int offset_in_cluster = offset_into_cluster(s, offset);
> +
> +    switch (cluster_type) {

[...]

> +    default:
> +        g_assert_not_reached();
> +        /*
> +         * QCOW2_CLUSTER_ZERO_PLAIN and QCOW2_CLUSTER_ZERO_ALLOC handled
> +         * in qcow2_co_preadv_part

Hmm, I’d still add them explicitly as cases and put their own
g_assert_not_reach() there.

> +         */
> +    }
> +
> +    g_assert_not_reached();
> +
> +    return -EIO;

Maybe abort()ing instead of g_assert_not_reach() would save you from
having to return here?

Max


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