From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57228) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y9KY6-0003tB-GG for qemu-devel@nongnu.org; Thu, 08 Jan 2015 16:23:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y9KY1-0003Ql-Kg for qemu-devel@nongnu.org; Thu, 08 Jan 2015 16:23:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60485) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y9KY1-0003Qd-AD for qemu-devel@nongnu.org; Thu, 08 Jan 2015 16:23:17 -0500 Message-ID: <54AEF543.5000303@redhat.com> Date: Thu, 08 Jan 2015 16:23:15 -0500 From: John Snow MIME-Version: 1.0 References: <1418307457-25996-1-git-send-email-vsementsov@parallels.com> <1418307457-25996-7-git-send-email-vsementsov@parallels.com> In-Reply-To: <1418307457-25996-7-git-send-email-vsementsov@parallels.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 6/9] block-migration: tiny refactoring List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org Cc: kwolf@redhat.com, den@openvz.org, stefanha@redhat.com On 12/11/2014 09:17 AM, Vladimir Sementsov-Ogievskiy wrote: > Add blk_create and blk_free to remove code duplicates. Otherwise, > duplicates will rise in the following patches because of BlkMigBlock > sturcture extendin. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block-migration.c | 56 +++++++++++++++++++++++++++++-------------------------- > 1 file changed, 30 insertions(+), 26 deletions(-) > > diff --git a/block-migration.c b/block-migration.c > index 5b4aa0f..d0c825f 100644 > --- a/block-migration.c > +++ b/block-migration.c > @@ -113,6 +113,30 @@ static void blk_mig_unlock(void) > qemu_mutex_unlock(&block_mig_state.lock); > } > > +/* Only allocating and initializing structure fields, not copying any data. */ > + > +static BlkMigBlock *blk_create(BlkMigDevState *bmds, int64_t sector, > + int nr_sectors) > +{ > + BlkMigBlock *blk = g_new(BlkMigBlock, 1); > + blk->buf = g_malloc(BLOCK_SIZE); > + blk->bmds = bmds; > + blk->sector = sector; > + blk->nr_sectors = nr_sectors; > + > + blk->iov.iov_base = blk->buf; > + blk->iov.iov_len = nr_sectors * BDRV_SECTOR_SIZE; > + qemu_iovec_init_external(&blk->qiov, &blk->iov, 1); > + > + return blk; > +} > + > +static void blk_free(BlkMigBlock *blk) > +{ > + g_free(blk->buf); > + g_free(blk); > +} > + > /* Must run outside of the iothread lock during the bulk phase, > * or the VM will stall. > */ > @@ -285,15 +309,7 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds) > nr_sectors = total_sectors - cur_sector; > } > > - blk = g_new(BlkMigBlock, 1); > - blk->buf = g_malloc(BLOCK_SIZE); > - blk->bmds = bmds; > - blk->sector = cur_sector; > - blk->nr_sectors = nr_sectors; > - > - blk->iov.iov_base = blk->buf; > - blk->iov.iov_len = nr_sectors * BDRV_SECTOR_SIZE; > - qemu_iovec_init_external(&blk->qiov, &blk->iov, 1); > + blk = blk_create(bmds, cur_sector, nr_sectors); > > blk_mig_lock(); > block_mig_state.submitted++; > @@ -467,17 +483,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds, > } else { > nr_sectors = BDRV_SECTORS_PER_DIRTY_CHUNK; > } > - blk = g_new(BlkMigBlock, 1); > - blk->buf = g_malloc(BLOCK_SIZE); > - blk->bmds = bmds; > - blk->sector = sector; > - blk->nr_sectors = nr_sectors; > + blk = blk_create(bmds, sector, nr_sectors); > > if (is_async) { > - blk->iov.iov_base = blk->buf; > - blk->iov.iov_len = nr_sectors * BDRV_SECTOR_SIZE; > - qemu_iovec_init_external(&blk->qiov, &blk->iov, 1); > - I suppose in the (!is_async) branch we don't reference iov/qiov again, but the functional difference caught my eye. It used to only be called under the "is_async" branch, but now is going to be executed unconditionally. Is that fine? > blk->aiocb = bdrv_aio_readv(bmds->bs, sector, &blk->qiov, > nr_sectors, blk_mig_read_cb, blk); > > @@ -492,8 +500,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds, > } > blk_send(f, blk); > > - g_free(blk->buf); > - g_free(blk); > + blk_free(blk); > } > > bdrv_reset_dirty_bitmap(bmds->bs, bmds->dirty_bitmap, sector, > @@ -508,8 +515,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds, > > error: > DPRINTF("Error reading sector %" PRId64 "\n", sector); > - g_free(blk->buf); > - g_free(blk); > + blk_free(blk); > return ret; > } > > @@ -560,8 +566,7 @@ static int flush_blks(QEMUFile *f) > blk_send(f, blk); > blk_mig_lock(); > > - g_free(blk->buf); > - g_free(blk); > + blk_free(blk); > > block_mig_state.read_done--; > block_mig_state.transferred++; > @@ -612,8 +617,7 @@ static void blk_mig_cleanup(void) > > while ((blk = QSIMPLEQ_FIRST(&block_mig_state.blk_list)) != NULL) { > QSIMPLEQ_REMOVE_HEAD(&block_mig_state.blk_list, entry); > - g_free(blk->buf); > - g_free(blk); > + blk_free(blk); > } > blk_mig_unlock(); > } > Reviewed-by: John Snow