Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> wrote:
> Move enabled_bitmaps and finish_lock, which are part of incoming state
> to DirtyBitmapLoadState, and make static global variable to store state
> instead of static local one.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
> migration/block-dirty-bitmap.c | 77 +++++++++++++++++++---------------
> 1 file changed, 43 insertions(+), 34 deletions(-)
>
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 7eafface61..281d20f41d 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -125,6 +125,13 @@ typedef struct DirtyBitmapMigState {
> BlockDriverState *prev_bs;
> BdrvDirtyBitmap *prev_bitmap;
> } DirtyBitmapMigState;
> +static DirtyBitmapMigState dirty_bitmap_mig_state;
Missing new line.
> +
> +typedef struct DirtyBitmapLoadBitmapState {
> + BlockDriverState *bs;
> + BdrvDirtyBitmap *bitmap;
> + bool migrated;
> +} DirtyBitmapLoadBitmapState;
>
> typedef struct DirtyBitmapLoadState {
> uint32_t flags;
> @@ -132,21 +139,15 @@ typedef struct DirtyBitmapLoadState {
> char bitmap_name[256];
> BlockDriverState *bs;
> BdrvDirtyBitmap *bitmap;
> -} DirtyBitmapLoadState;
>
> -static DirtyBitmapMigState dirty_bitmap_mig_state;
> -
> -typedef struct DirtyBitmapLoadBitmapState {
> - BlockDriverState *bs;
> - BdrvDirtyBitmap *bitmap;
> - bool migrated;
> -} DirtyBitmapLoadBitmapState;
> -static GSList *enabled_bitmaps;
> -QemuMutex finish_lock;
> + GSList *enabled_bitmaps;
> + QemuMutex finish_lock;
> +} DirtyBitmapLoadState;
> +static DirtyBitmapLoadState dbm_load_state;
You move two global variables to an struct (good)
But you create a even bigger global variable (i.e. state that was not
shared before.)
> /* First occurrence of this bitmap. It should be created if doesn't exist */
> -static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
> +static int dirty_bitmap_load_start(QEMUFile *f)
> {
> + DirtyBitmapLoadState *s = &dbm_load_state;
You create a local alias.
> Error *local_err = NULL;
> uint32_t granularity = qemu_get_be32(f);
> uint8_t flags = qemu_get_byte(f);
> @@ -482,7 +484,8 @@ static int dirty_bitmap_load_start(QEMUFile *f,
> DirtyBitmapLoadState *s)
> b->bs = s->bs;
> b->bitmap = s->bitmap;
> b->migrated = false;
> - enabled_bitmaps = g_slist_prepend(enabled_bitmaps, b);
> + dbm_load_state.enabled_bitmaps =
> + g_slist_prepend(dbm_load_state.enabled_bitmaps, b);
And then you access it using the global variable?
> -static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
> +static void dirty_bitmap_load_complete(QEMUFile *f)
> {
> + DirtyBitmapLoadState *s = &dbm_load_state;
Exactly the same on this function.
I still don't understand why you are removing the pass as parameters of
this function.
> - static DirtyBitmapLoadState s;
Aha, this is why you are moving it as a global.
But, why can't you put this inside dirty_bitmap_mig_state? Then you:
a- don't have to have "yet" another global variable
b- you can clean it up on save_cleanup handler.
not related to this patch, but to the file in general:
static void dirty_bitmap_save_cleanup(void *opaque)
{
dirty_bitmap_mig_cleanup();
}
We have opaque here, that we can do:
DirtyBitmapMigBitmapState *dbms = opaque;
And then pass dbms to dirty_bitmap_mig_cleanup().
/* Called with iothread lock taken. */
static void dirty_bitmap_mig_cleanup(void)
{
DirtyBitmapMigBitmapState *dbms;
while ((dbms = QSIMPLEQ_FIRST(&dirty_bitmap_mig_state.dbms_list)) != NULL) {
QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
bdrv_unref(dbms->bs);
g_free(dbms);
}
}
Because here we just use the global variable.
Later, Juan.
