| Message ID | 1453311539-1193-14-git-send-email-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/20/2016 10:38 AM, 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 > and the other two callers are easily converted todo so. s/todo/to do/ > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > block/qcow.c | 31 ++++++++++--------------------- > 1 file changed, 10 insertions(+), 21 deletions(-) > > @@ -453,13 +447,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++) { As long as you're touching near here, is it worth adding a space before '('? > @@ -672,7 +665,6 @@ 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; > @@ -715,18 +707,15 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, > 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, > n, true, &err) < 0) { > error_free(err); > ret = -EIO; > break; > } > - src_buf = cluster_data; cluster_data is now unused; you've got a wasted g_malloc0() here and g_free() later on.
On Mon, Feb 08, 2016 at 01:30:10PM -0700, Eric Blake wrote: > On 01/20/2016 10:38 AM, 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 > > and the other two callers are easily converted todo so. > > s/todo/to do/ > > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > block/qcow.c | 31 ++++++++++--------------------- > > 1 file changed, 10 insertions(+), 21 deletions(-) > > > > > @@ -453,13 +447,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++) { > > As long as you're touching near here, is it worth adding a space before '('? > > > @@ -672,7 +665,6 @@ 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; > > @@ -715,18 +707,15 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, > > 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, > > n, true, &err) < 0) { > > error_free(err); > > ret = -EIO; > > break; > > } > > - src_buf = cluster_data; > > cluster_data is now unused; you've got a wasted g_malloc0() here and > g_free() later on. Oh, good point. Regards, Daniel
diff --git a/block/qcow.c b/block/qcow.c index 635085e..5a4f310 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -293,12 +293,9 @@ static int qcow_set_key(BlockDriverState *bs, const char *key) return 0; } -/* The crypt function is compatible with the linux cryptoloop - algorithm for < 4 GB images. NOTE: out_buf == in_buf is - supported */ 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]; @@ -317,14 +314,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); } @@ -332,8 +327,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; } @@ -453,13 +447,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; @@ -637,7 +630,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; } @@ -672,7 +665,6 @@ 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; @@ -715,18 +707,15 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, 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, 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);
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 and the other two callers are easily converted todo so. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- block/qcow.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-)