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

[John Snow]

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 <address@hidden>
> > > ---
> > >   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!"