| Message ID | cec353c1d29bce731bc0dd84db7642dbbdbfe9b6.1316782367.git.quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
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<quintela@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. Regards, Anthony Liguori > > static NotifierList migration_state_notifiers = > NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); > @@ -136,37 +142,34 @@ void do_info_migrate(Monitor *mon, QObject **ret_data) > { > QDict *qdict; > > - if (current_migration) { > - > - switch (current_migration->state) { > - case MIG_STATE_NONE: > - /* no migration has happened ever */ > - break; > - case MIG_STATE_ACTIVE: > - qdict = qdict_new(); > - qdict_put(qdict, "status", qstring_from_str("active")); > - > - migrate_put_status(qdict, "ram", ram_bytes_transferred(), > - ram_bytes_remaining(), ram_bytes_total()); > - > - if (blk_mig_active()) { > - migrate_put_status(qdict, "disk", blk_mig_bytes_transferred(), > - blk_mig_bytes_remaining(), > - blk_mig_bytes_total()); > - } > - > - *ret_data = QOBJECT(qdict); > - break; > - case MIG_STATE_COMPLETED: > - *ret_data = qobject_from_jsonf("{ 'status': 'completed' }"); > - break; > - case MIG_STATE_ERROR: > - *ret_data = qobject_from_jsonf("{ 'status': 'failed' }"); > - break; > - case MIG_STATE_CANCELLED: > - *ret_data = qobject_from_jsonf("{ 'status': 'cancelled' }"); > - break; > + switch (current_migration->state) { > + case MIG_STATE_NONE: > + /* no migration has happened ever */ > + break; > + case MIG_STATE_ACTIVE: > + qdict = qdict_new(); > + qdict_put(qdict, "status", qstring_from_str("active")); > + > + migrate_put_status(qdict, "ram", ram_bytes_transferred(), > + ram_bytes_remaining(), ram_bytes_total()); > + > + if (blk_mig_active()) { > + migrate_put_status(qdict, "disk", blk_mig_bytes_transferred(), > + blk_mig_bytes_remaining(), > + blk_mig_bytes_total()); > } > + > + *ret_data = QOBJECT(qdict); > + break; > + case MIG_STATE_COMPLETED: > + *ret_data = qobject_from_jsonf("{ 'status': 'completed' }"); > + break; > + case MIG_STATE_ERROR: > + *ret_data = qobject_from_jsonf("{ 'status': 'failed' }"); > + break; > + case MIG_STATE_CANCELLED: > + *ret_data = qobject_from_jsonf("{ 'status': 'cancelled' }"); > + break; > } > } > > @@ -357,11 +360,7 @@ void remove_migration_state_change_notifier(Notifier *notify) > > int get_migration_state(void) > { > - if (current_migration) { > - return current_migration->state; > - } else { > - return MIG_STATE_ERROR; > - } > + return current_migration->state; > } > > void migrate_fd_connect(MigrationState *s) > @@ -386,28 +385,23 @@ void migrate_fd_connect(MigrationState *s) > migrate_fd_put_ready(s); > } > > -static MigrationState *migrate_create_state(Monitor *mon, > - int64_t bandwidth_limit, > - int detach, int blk, int inc) > +static void migrate_init_state(Monitor *mon, int64_t bandwidth_limit, > + int detach, int blk, int inc) > { > - MigrationState *s = g_malloc0(sizeof(*s)); > - > - s->blk = blk; > - s->shared = inc; > - s->mon = NULL; > - s->bandwidth_limit = bandwidth_limit; > - s->state = MIG_STATE_NONE; > + memset(current_migration, 0, sizeof(current_migration)); > + current_migration->blk = blk; > + current_migration->shared = inc; > + current_migration->mon = NULL; > + current_migration->bandwidth_limit = bandwidth_limit; > + current_migration->state = MIG_STATE_NONE; > > if (!detach) { > - migrate_fd_monitor_suspend(s, mon); > + migrate_fd_monitor_suspend(current_migration, mon); > } > - > - return s; > } > > int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) > { > - MigrationState *s = NULL; > const char *p; > int detach = qdict_get_try_bool(qdict, "detach", 0); > int blk = qdict_get_try_bool(qdict, "blk", 0); > @@ -415,8 +409,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) > const char *uri = qdict_get_str(qdict, "uri"); > int ret; > > - if (current_migration&& > - current_migration->state == MIG_STATE_ACTIVE) { > + if (current_migration->state == MIG_STATE_ACTIVE) { > monitor_printf(mon, "migration already in progress\n"); > return -1; > } > @@ -425,43 +418,35 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) > return -1; > } > > - s = migrate_create_state(mon, max_throttle, detach, blk, inc); > + migrate_init_state(mon, max_throttle, detach, blk, inc); > > if (strstart(uri, "tcp:",&p)) { > - ret = tcp_start_outgoing_migration(s, p); > + ret = tcp_start_outgoing_migration(current_migration, p); > #if !defined(WIN32) > } else if (strstart(uri, "exec:",&p)) { > - ret = exec_start_outgoing_migration(s, p); > + ret = exec_start_outgoing_migration(current_migration, p); > } else if (strstart(uri, "unix:",&p)) { > - ret = unix_start_outgoing_migration(s, p); > + ret = unix_start_outgoing_migration(current_migration, p); > } else if (strstart(uri, "fd:",&p)) { > - ret = fd_start_outgoing_migration(s, p); > + ret = fd_start_outgoing_migration(current_migration, p); > #endif > } else { > monitor_printf(mon, "unknown migration protocol: %s\n", uri); > - ret = -EINVAL; > - goto free_migrate_state; > + return -EINVAL; > } > > if (ret< 0) { > monitor_printf(mon, "migration failed\n"); > - goto free_migrate_state; > + return ret; > } > > - g_free(current_migration); > - current_migration = s; > notifier_list_notify(&migration_state_notifiers, NULL); > return 0; > -free_migrate_state: > - g_free(s); > - return -1; > } > > int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data) > { > - if (current_migration) { > - migrate_fd_cancel(current_migration); > - } > + migrate_fd_cancel(current_migration); > return 0; > } > > @@ -475,10 +460,7 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data) > } > max_throttle = d; > > - if (current_migration) { > - qemu_file_set_rate_limit(current_migration->file, max_throttle); > - } > - > + qemu_file_set_rate_limit(current_migration->file, max_throttle); > return 0; > } >
Anthony Liguori <anthony@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<quintela@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, and it also avoids having to have new variables (max_throotle) just because we don't have a migration state handy. 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.
On 10/04/2011 09:49 AM, Juan Quintela wrote: > Anthony Liguori<anthony@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<quintela@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. >
Anthony Liguori <anthony@codemonkey.ws> wrote: > On 10/04/2011 09:49 AM, Juan Quintela wrote: >> Anthony Liguori<anthony@codemonkey.ws> wrote: >>> On 09/23/2011 07:57 AM, Juan Quintela wrote: [ much more stuff ] >> 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. I understand the singleton problem, but the reason to put STATE_NONE is not for that O:-) (it just happens that we only have a migration now). Why I want it? Just now, the only thing that we are "setting" for a migration before it starts is the "bandwidth". I see the future when migration becomes something like: migration_set_speed .... migration_set_target .... migration_set_<whatever else> migrate as you can see, we are "preparing" migration, and we don't have a "STATE" now that describes this state, migration not started, but we want to prepare it. Perhaps a better name that STATE_NONE is in order. I got NONE in the sense that "migration" has not been attemted yet. Later, Juan.
On 10/05/2011 10:25 PM, Juan Quintela wrote: > Anthony Liguori<anthony@codemonkey.ws> wrote: >> On 10/04/2011 09:49 AM, Juan Quintela wrote: >>> Anthony Liguori<anthony@codemonkey.ws> wrote: >>>> On 09/23/2011 07:57 AM, Juan Quintela wrote: > [ much more stuff ] > >>> 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. > I understand the singleton problem, but the reason to put STATE_NONE is > not for that O:-) (it just happens that we only have a migration now). > > Why I want it? > > Just now, the only thing that we are "setting" for a migration before it > starts is the "bandwidth". I see the future when migration becomes > something like: > > migration_set_speed .... > migration_set_target .... > migration_set_<whatever else> > migrate > > as you can see, we are "preparing" migration, and we don't have a > "STATE" now that describes this state, migration not started, but we > want to prepare it. > > Perhaps a better name that STATE_NONE is in order. I got NONE in the > sense that "migration" has not been attemted yet. How about STATE_SETUP or STATE_PREPARE ? Cheers, Orit > Later, Juan. >
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; static NotifierList migration_state_notifiers = NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); @@ -136,37 +142,34 @@ void do_info_migrate(Monitor *mon, QObject **ret_data) { QDict *qdict; - if (current_migration) { - - switch (current_migration->state) { - case MIG_STATE_NONE: - /* no migration has happened ever */ - break; - case MIG_STATE_ACTIVE: - qdict = qdict_new(); - qdict_put(qdict, "status", qstring_from_str("active")); - - migrate_put_status(qdict, "ram", ram_bytes_transferred(), - ram_bytes_remaining(), ram_bytes_total()); - - if (blk_mig_active()) { - migrate_put_status(qdict, "disk", blk_mig_bytes_transferred(), - blk_mig_bytes_remaining(), - blk_mig_bytes_total()); - } - - *ret_data = QOBJECT(qdict); - break; - case MIG_STATE_COMPLETED: - *ret_data = qobject_from_jsonf("{ 'status': 'completed' }"); - break; - case MIG_STATE_ERROR: - *ret_data = qobject_from_jsonf("{ 'status': 'failed' }"); - break; - case MIG_STATE_CANCELLED: - *ret_data = qobject_from_jsonf("{ 'status': 'cancelled' }"); - break; + switch (current_migration->state) { + case MIG_STATE_NONE: + /* no migration has happened ever */ + break; + case MIG_STATE_ACTIVE: + qdict = qdict_new(); + qdict_put(qdict, "status", qstring_from_str("active")); + + migrate_put_status(qdict, "ram", ram_bytes_transferred(), + ram_bytes_remaining(), ram_bytes_total()); + + if (blk_mig_active()) { + migrate_put_status(qdict, "disk", blk_mig_bytes_transferred(), + blk_mig_bytes_remaining(), + blk_mig_bytes_total()); } + + *ret_data = QOBJECT(qdict); + break; + case MIG_STATE_COMPLETED: + *ret_data = qobject_from_jsonf("{ 'status': 'completed' }"); + break; + case MIG_STATE_ERROR: + *ret_data = qobject_from_jsonf("{ 'status': 'failed' }"); + break; + case MIG_STATE_CANCELLED: + *ret_data = qobject_from_jsonf("{ 'status': 'cancelled' }"); + break; } } @@ -357,11 +360,7 @@ void remove_migration_state_change_notifier(Notifier *notify) int get_migration_state(void) { - if (current_migration) { - return current_migration->state; - } else { - return MIG_STATE_ERROR; - } + return current_migration->state; } void migrate_fd_connect(MigrationState *s) @@ -386,28 +385,23 @@ void migrate_fd_connect(MigrationState *s) migrate_fd_put_ready(s); } -static MigrationState *migrate_create_state(Monitor *mon, - int64_t bandwidth_limit, - int detach, int blk, int inc) +static void migrate_init_state(Monitor *mon, int64_t bandwidth_limit, + int detach, int blk, int inc) { - MigrationState *s = g_malloc0(sizeof(*s)); - - s->blk = blk; - s->shared = inc; - s->mon = NULL; - s->bandwidth_limit = bandwidth_limit; - s->state = MIG_STATE_NONE; + memset(current_migration, 0, sizeof(current_migration)); + current_migration->blk = blk; + current_migration->shared = inc; + current_migration->mon = NULL; + current_migration->bandwidth_limit = bandwidth_limit; + current_migration->state = MIG_STATE_NONE; if (!detach) { - migrate_fd_monitor_suspend(s, mon); + migrate_fd_monitor_suspend(current_migration, mon); } - - return s; } int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) { - MigrationState *s = NULL; const char *p; int detach = qdict_get_try_bool(qdict, "detach", 0); int blk = qdict_get_try_bool(qdict, "blk", 0); @@ -415,8 +409,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) const char *uri = qdict_get_str(qdict, "uri"); int ret; - if (current_migration && - current_migration->state == MIG_STATE_ACTIVE) { + if (current_migration->state == MIG_STATE_ACTIVE) { monitor_printf(mon, "migration already in progress\n"); return -1; } @@ -425,43 +418,35 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) return -1; } - s = migrate_create_state(mon, max_throttle, detach, blk, inc); + migrate_init_state(mon, max_throttle, detach, blk, inc); if (strstart(uri, "tcp:", &p)) { - ret = tcp_start_outgoing_migration(s, p); + ret = tcp_start_outgoing_migration(current_migration, p); #if !defined(WIN32) } else if (strstart(uri, "exec:", &p)) { - ret = exec_start_outgoing_migration(s, p); + ret = exec_start_outgoing_migration(current_migration, p); } else if (strstart(uri, "unix:", &p)) { - ret = unix_start_outgoing_migration(s, p); + ret = unix_start_outgoing_migration(current_migration, p); } else if (strstart(uri, "fd:", &p)) { - ret = fd_start_outgoing_migration(s, p); + ret = fd_start_outgoing_migration(current_migration, p); #endif } else { monitor_printf(mon, "unknown migration protocol: %s\n", uri); - ret = -EINVAL; - goto free_migrate_state; + return -EINVAL; } if (ret < 0) { monitor_printf(mon, "migration failed\n"); - goto free_migrate_state; + return ret; } - g_free(current_migration); - current_migration = s; notifier_list_notify(&migration_state_notifiers, NULL); return 0; -free_migrate_state: - g_free(s); - return -1; } int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data) { - if (current_migration) { - migrate_fd_cancel(current_migration); - } + migrate_fd_cancel(current_migration); return 0; } @@ -475,10 +460,7 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data) } max_throttle = d; - if (current_migration) { - qemu_file_set_rate_limit(current_migration->file, max_throttle); - } - + qemu_file_set_rate_limit(current_migration->file, max_throttle); return 0; }
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 <quintela@redhat.com> --- migration.c | 126 +++++++++++++++++++++++++--------------------------------- 1 files changed, 54 insertions(+), 72 deletions(-)