Re: [Qemu-devel] [PATCH v2 13/17] qcow: make encrypt_sectors encrypt in place

From: "Daniel P. Berrange" <berrange@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Fam Zheng <famz@redhat.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2 13/17] qcow: make encrypt_sectors encrypt in place
Date: Tue, 9 Feb 2016 12:33:19 +0000	[thread overview]
Message-ID: <20160209123319.GG24614@redhat.com> (raw)
In-Reply-To: <56B8FAD2.2090404@redhat.com>

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|