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

[John Snow <jsnow@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 <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?
>

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!"