On 03.01.2017 19:27, Daniel P. Berrange wrote: > Instead of requiring separate input/output buffers for > encrypting data, change encrypt_sectors() to assume > use of a single buffer, encrypting in place. One current > caller all uses the same buffer for input/output already
-all?
> and the other two callers are easily converted todo so.
s/todo/to do/
>
> Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
> ---
> block/qcow.c | 36 +++++++++++-------------------------
> 1 file changed, 11 insertions(+), 25 deletions(-)
>
> diff --git a/block/qcow.c b/block/qcow.c
> index 8133fda..bc9fa2f 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -310,11 +310,10 @@ static int qcow_set_key(BlockDriverState *bs, const
> char *key)
> }
>
> /* The crypt function is compatible with the linux cryptoloop
> - algorithm for < 4 GB images. NOTE: out_buf == in_buf is
> - supported */
> + algorithm for < 4 GB images. */
> static int encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
> - uint8_t *out_buf, const uint8_t *in_buf,
> - int nb_sectors, bool enc, Error **errp)
> + uint8_t *buf, int nb_sectors, bool enc,
> + Error **errp)
> {
> union {
> uint64_t ll[2];
> @@ -333,14 +332,12 @@ static int encrypt_sectors(BDRVQcowState *s, int64_t
> sector_num,
> }
> if (enc) {
> ret = qcrypto_cipher_encrypt(s->cipher,
> - in_buf,
> - out_buf,
> + buf, buf,
> 512,
> errp);
> } else {
> ret = qcrypto_cipher_decrypt(s->cipher,
> - in_buf,
> - out_buf,
> + buf, buf,
> 512,
> errp);
> }
> @@ -348,8 +345,7 @@ static int encrypt_sectors(BDRVQcowState *s, int64_t
> sector_num,
> return -1;
> }
> sector_num++;
> - in_buf += 512;
> - out_buf += 512;
> + buf += 512;
> }
> return 0;
> }
> @@ -469,13 +465,12 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
> uint64_t start_sect;
> assert(s->cipher);
> start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
> - memset(s->cluster_data + 512, 0x00, 512);
> + memset(s->cluster_data, 0x00, 512);
> for(i = 0; i < s->cluster_sectors; i++) {
> if (i < n_start || i >= n_end) {
> Error *err = NULL;
> if (encrypt_sectors(s, start_sect + i,
> - s->cluster_data,
> - s->cluster_data + 512, 1,
> + s->cluster_data, 1,
> true, &err) < 0) {
> error_free(err);
> errno = EIO;
After the first iteration of the surrounding for () loop,
s->cluster_data is unlikely to still be filled with zeros -- but I
suspect the code intended to always write encrypted zeros.
> @@ -653,7 +648,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState
> *bs, int64_t sector_num,
> }
> if (bs->encrypted) {
> assert(s->cipher);
> - if (encrypt_sectors(s, sector_num, buf, buf,
> + if (encrypt_sectors(s, sector_num, buf,
> n, false, &err) < 0) {
> goto fail;
> }
> @@ -688,9 +683,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState
> *bs, int64_t sector_num,
> BDRVQcowState *s = bs->opaque;
> int index_in_cluster;
> uint64_t cluster_offset;
> - const uint8_t *src_buf;
> int ret = 0, n;
> - uint8_t *cluster_data = NULL;
> struct iovec hd_iov;
> QEMUIOVector hd_qiov;
> uint8_t *buf;
> @@ -728,21 +721,15 @@ static coroutine_fn int qcow_co_writev(BlockDriverState
> *bs, int64_t sector_num,
> if (bs->encrypted) {
> Error *err = NULL;
> assert(s->cipher);
> - if (!cluster_data) {
> - cluster_data = g_malloc0(s->cluster_size);
> - }
> - if (encrypt_sectors(s, sector_num, cluster_data, buf,
> + if (encrypt_sectors(s, sector_num, buf,
If qiov->niov == 1, buf is not copied from the I/O vector but just the
I/O vector base itself. Then, this will modify the data pointed to by
that vector. I don't think that is a good idea.
Max
> n, true, &err) < 0) {
> error_free(err);
> ret = -EIO;
> break;
> }
> - src_buf = cluster_data;
> - } else {
> - src_buf = buf;
> }
>
> - hd_iov.iov_base = (void *)src_buf;
> + hd_iov.iov_base = (void *)buf;
> hd_iov.iov_len = n * 512;
> qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
> qemu_co_mutex_unlock(&s->lock);
> @@ -764,7 +751,6 @@ static coroutine_fn int qcow_co_writev(BlockDriverState
> *bs, int64_t sector_num,
> if (qiov->niov > 1) {
> qemu_vfree(orig_buf);
> }
> - g_free(cluster_data);
>
> return ret;
> }
>
signature.asc
Description: OpenPGP digital signature
