Re: [Qemu-devel] [PATCH 6/9] block-migration: tiny refactoring

[Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>]

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 <vsementsov@parallels.com>
>> ---
>>   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?