Patchwork [14/22] migration: Remove get_status() accessor

login
register
mail settings
Submitter Juan Quintela
Date Feb. 23, 2011, 12:44 a.m.
Message ID <03353a90bb08ef0f82e1caa0a294524d0ea2baa0.1298421307.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/84031/
State New
Headers show

Comments

Juan Quintela - Feb. 23, 2011, 12:44 a.m.
It is only used inside migration.c, and fields on that struct are
accessed all around the place on that file.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |   16 +++++-----------
 migration.h |    1 -
 2 files changed, 5 insertions(+), 12 deletions(-)
Yoshiaki Tamura - Feb. 23, 2011, 8:42 a.m.
2011/2/23 Juan Quintela <quintela@redhat.com>:
> It is only used inside migration.c, and fields on that struct are
> accessed all around the place on that file.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |   16 +++++-----------
>  migration.h |    1 -
>  2 files changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index dfe6a96..2b873fa 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -90,7 +90,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     int ret;
>
>     if (current_migration &&
> -        current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
> +        current_migration->state == MIG_STATE_ACTIVE) {
>         monitor_printf(mon, "migration already in progress\n");
>         return -1;
>     }
> @@ -135,7 +135,7 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>     MigrationState *s = current_migration;
>
> -    if (s && s->get_status(s) == MIG_STATE_ACTIVE)
> +    if (s && s->state == MIG_STATE_ACTIVE)
>         s->cancel(s);
>
>     return 0;
> @@ -234,7 +234,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
>     if (current_migration) {
>         MigrationState *s = current_migration;
>
> -        switch (s->get_status(current_migration)) {
> +        switch (s->state) {
>         case MIG_STATE_NONE:
>             /* no migration has happened ever */
>             break;
> @@ -375,7 +375,7 @@ static void migrate_fd_put_ready(void *opaque)
>         } else {
>             migrate_fd_completed(s);
>         }
> -        if (s->get_status(s) != MIG_STATE_COMPLETED) {
> +        if (s->state != MIG_STATE_COMPLETED) {
>             if (old_vm_running) {
>                 vm_start();
>             }
> @@ -383,11 +383,6 @@ static void migrate_fd_put_ready(void *opaque)
>     }
>  }
>
> -static int migrate_fd_get_status(MigrationState *s)
> -{
> -    return s->state;
> -}
> -
>  static void migrate_fd_cancel(MigrationState *s)
>  {
>     if (s->state != MIG_STATE_ACTIVE)
> @@ -442,7 +437,7 @@ void remove_migration_state_change_notifier(Notifier *notify)
>  int get_migration_state(void)
>  {
>     if (current_migration) {
> -        return migrate_fd_get_status(current_migration);
> +        return current_migration->state;
>     } else {
>         return MIG_STATE_ERROR;
>     }
> @@ -477,7 +472,6 @@ static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limi
>     MigrationState *s = qemu_mallocz(sizeof(*s));
>
>     s->cancel = migrate_fd_cancel;
> -    s->get_status = migrate_fd_get_status;
>     s->blk = blk;
>     s->shared = inc;
>     s->mon = NULL;
> diff --git a/migration.h b/migration.h
> index 5455d8b..58a6e06 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -37,7 +37,6 @@ struct MigrationState
>     int (*close)(MigrationState*);
>     int (*write)(MigrationState*, const void *, size_t);
>     void (*cancel)(MigrationState *s);
> -    int (*get_status)(MigrationState *s);
>     void *opaque;
>     int blk;
>     int shared;

I agree to access s->state directly inside of migration.c, but I
disagree to remove get_status() accessor right away.  We don't
have strong motivations for doing that AFAIK.

Yoshi

> --
> 1.7.4
>
>
>
Juan Quintela - Feb. 23, 2011, 9:18 a.m.
Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>> It is only used inside migration.c, and fields on that struct are
>> accessed all around the place on that file.

>
> I agree to access s->state directly inside of migration.c, but I
> disagree to remove get_status() accessor right away.  We don't
> have strong motivations for doing that AFAIK.

Only user outside of migration.c was ui/spice-core.c, and it just wanted
to know if migration has finished at all.

At this point I was trying to isolate what other parts of MigrationState
are used externally.  That way, it gets easier to change that later.

At this point, only things used outside of migration.c are:
- write, clase, get_error: trivial to fix, just add setters for them.
- fd: that is not enterely trivial to fix.

Later, Juan.

Patch

diff --git a/migration.c b/migration.c
index dfe6a96..2b873fa 100644
--- a/migration.c
+++ b/migration.c
@@ -90,7 +90,7 @@  int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     int ret;

     if (current_migration &&
-        current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
+        current_migration->state == MIG_STATE_ACTIVE) {
         monitor_printf(mon, "migration already in progress\n");
         return -1;
     }
@@ -135,7 +135,7 @@  int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     MigrationState *s = current_migration;

-    if (s && s->get_status(s) == MIG_STATE_ACTIVE)
+    if (s && s->state == MIG_STATE_ACTIVE)
         s->cancel(s);

     return 0;
@@ -234,7 +234,7 @@  void do_info_migrate(Monitor *mon, QObject **ret_data)
     if (current_migration) {
         MigrationState *s = current_migration;

-        switch (s->get_status(current_migration)) {
+        switch (s->state) {
         case MIG_STATE_NONE:
             /* no migration has happened ever */
             break;
@@ -375,7 +375,7 @@  static void migrate_fd_put_ready(void *opaque)
         } else {
             migrate_fd_completed(s);
         }
-        if (s->get_status(s) != MIG_STATE_COMPLETED) {
+        if (s->state != MIG_STATE_COMPLETED) {
             if (old_vm_running) {
                 vm_start();
             }
@@ -383,11 +383,6 @@  static void migrate_fd_put_ready(void *opaque)
     }
 }

-static int migrate_fd_get_status(MigrationState *s)
-{
-    return s->state;
-}
-
 static void migrate_fd_cancel(MigrationState *s)
 {
     if (s->state != MIG_STATE_ACTIVE)
@@ -442,7 +437,7 @@  void remove_migration_state_change_notifier(Notifier *notify)
 int get_migration_state(void)
 {
     if (current_migration) {
-        return migrate_fd_get_status(current_migration);
+        return current_migration->state;
     } else {
         return MIG_STATE_ERROR;
     }
@@ -477,7 +472,6 @@  static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limi
     MigrationState *s = qemu_mallocz(sizeof(*s));

     s->cancel = migrate_fd_cancel;
-    s->get_status = migrate_fd_get_status;
     s->blk = blk;
     s->shared = inc;
     s->mon = NULL;
diff --git a/migration.h b/migration.h
index 5455d8b..58a6e06 100644
--- a/migration.h
+++ b/migration.h
@@ -37,7 +37,6 @@  struct MigrationState
     int (*close)(MigrationState*);
     int (*write)(MigrationState*, const void *, size_t);
     void (*cancel)(MigrationState *s);
-    int (*get_status)(MigrationState *s);
     void *opaque;
     int blk;
     int shared;