On 10/04/2011 09:49 AM, Juan Quintela wrote:
Anthony Liguori<anth...@codemonkey.ws> wrote:
On 09/23/2011 07:57 AM, Juan Quintela wrote:
This cleans up a lot the code as we don't have to check anymore if
the variable is NULL or not.
We don't make it static, because when we integrate fault tolerance, we
can have several migrations at once.
Signed-off-by: Juan Quintela<quint...@redhat.com>
---
migration.c | 126 +++++++++++++++++++++++++---------------------------------
1 files changed, 54 insertions(+), 72 deletions(-)
diff --git a/migration.c b/migration.c
index 1cc21f0..c382383 100644
--- a/migration.c
+++ b/migration.c
@@ -34,7 +34,13 @@
/* Migration speed throttling */
static int64_t max_throttle = (32<< 20);
-static MigrationState *current_migration;
+/* When we add fault tolerance, we could have several
+ migrations at once. For now we don't need to add
+ dynamic creation of migration */
+static MigrationState current_migration_static = {
+ .state = MIG_STATE_NONE,
+};
+static MigrationState *current_migration =¤t_migration_static;
This doesn't make sense to me. We can have two migration states that
are both in the MIG_STATE_NONE? What does that actually mean?
I don't see the point in making migration state nullable via the state
member other than avoiding the if (s == NULL) check.
It avoids s==NULL checks,
In favor of s->state == MIG_STATE_NONE.
and it also avoids having to have new
variables (max_throotle) just because we don't have a migration state
handy.
The intention of the global variable is to set a default for new sessions. I
can imagine a world where if you had parallel migrations, you could either
toggle the default (the global variable) or the specific parameter within a
single migration session.
But it's not about features. It's a general reluctances to heavily modify the
code to rely on a global instead of passing the state around. Here's a pretty
good short write up of why this is a Bad Thing.
http://stackoverflow.com/questions/1392315/problems-with-singleton-pattern
Ultimately, MIG_STATE_NONE shouldn't be needed because we should not be relying
on a singleton accessor to get the MigrationState. A better cleanup would be to
further pass MigrationState between functions and eliminate the need for a
global at all.
Regards,
Anthony Liguori
Obviously the problem can't be the size (the variable is smaller that
all the code tests for NULL).
For me, it is easier to understand that we have an state (migration
never attempted) with the same states that the other migrations states.
Later, Juan.
