From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58223) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBRCZ-0007zt-RY for qemu-devel@nongnu.org; Wed, 14 Jan 2015 11:53:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YBRCW-0006Cp-IY for qemu-devel@nongnu.org; Wed, 14 Jan 2015 11:53:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41937) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBRCW-0006Cj-AS for qemu-devel@nongnu.org; Wed, 14 Jan 2015 11:53:48 -0500 Message-ID: <54B69F18.6060407@redhat.com> Date: Wed, 14 Jan 2015 11:53:44 -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> <54AEF543.5000303@redhat.com> <54B66075.5090109@parallels.com> In-Reply-To: <54B66075.5090109@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 01/14/2015 07:26 AM, Vladimir Sementsov-Ogievskiy wrote: > On 09.01.2015 00:23, John Snow wrote: >> >> >> 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? > It think it doesn't matter. I can add a parameter 'is_async' to > blk_create(), but what is worse - excess parameter or excess > initialization? And why not to initialize the whole structure in > blk_create() unconditionally? > If it's not a problem, leave it as-is. If I am not sure immediately myself, I like to ask questions. Your answer to the question can always be "Yes, that's fine!"