diff mbox

[v2,13/17] qcow: make encrypt_sectors encrypt in place

Message ID 1453311539-1193-14-git-send-email-berrange@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel P. Berrangé Jan. 20, 2016, 5:38 p.m. UTC
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(-)

Comments

Eric Blake Feb. 8, 2016, 8:30 p.m. UTC | #1
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.
Daniel P. Berrangé Feb. 9, 2016, 12:33 p.m. UTC | #2
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 mbox

Patch

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);