|
From: | Anthony Liguori |
Subject: | Re: [Qemu-devel] [PATCH 17/23] migration: make sure we always have a migration state |
Date: | Wed, 05 Oct 2011 15:02:58 -0500 |
User-agent: | Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.21) Gecko/20110831 Lightning/1.0b2 Thunderbird/3.1.13 |
On 10/04/2011 09:49 AM, Juan Quintela wrote:
Anthony Liguori<address@hidden> 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<address@hidden> --- 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-patternUltimately, 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.
[Prev in Thread] | Current Thread | [Next in Thread] |