24.01.2020 13:56, Juan Quintela wrote:
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.
Because dirty_bitmap_mig_state is source state, and new dbm_load_state is
destination state. So, at least [b] will not work...
Do you think it's good to combine both source and destination states into one
struct, and use opaque everywhere?
It will look like
DirtyBitmapMigSourceState *s = ((DirtyBitmapMigState *)opaque)->source_state;
in save functions
and
DirtyBitmapIncomingState *s = ((DirtyBitmapMigState *)opaque)->incoming_state;
in load function
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.
--
Best regards,
Vladimir
