Re: [Qemu-devel] [PATCH 05/23] migration: Refactor MigrationState creation

[Anthony Liguori <anthony@codemonkey.ws>]

On 09/23/2011 07:56 AM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> ---
>   migration-exec.c |   16 +---------------
>   migration-fd.c   |   16 +---------------
>   migration-tcp.c  |   15 +--------------
>   migration-unix.c |   15 +--------------
>   migration.c      |   29 +++++++++++++++++++++++++----
>   migration.h      |   11 +++--------
>   6 files changed, 32 insertions(+), 70 deletions(-)
>
> diff --git a/migration-exec.c b/migration-exec.c
> index 0ed5976..db11374 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -71,7 +71,7 @@ MigrationState *exec_start_outgoing_migration(Monitor *mon,
>       MigrationState *s;
>       FILE *f;
>
> -    s = g_malloc0(sizeof(*s));
> +    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);
>
>       f = popen(command, "w");
>       if (f == NULL) {
> @@ -92,20 +92,6 @@ MigrationState *exec_start_outgoing_migration(Monitor *mon,
>       s->close = exec_close;
>       s->get_error = file_errno;
>       s->write = file_write;
> -    s->cancel = migrate_fd_cancel;
> -    s->get_status = migrate_fd_get_status;
> -    s->release = migrate_fd_release;
> -
> -    s->blk = blk;
> -    s->shared = inc;
> -
> -    s->state = MIG_STATE_ACTIVE;
> -    s->mon = NULL;
> -    s->bandwidth_limit = bandwidth_limit;
> -
> -    if (!detach) {
> -        migrate_fd_monitor_suspend(s, mon);
> -    }
>
>       migrate_fd_connect(s);
>       return s;
> diff --git a/migration-fd.c b/migration-fd.c
> index e78fd4e..2235a2f 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -59,7 +59,7 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,
>   {
>       MigrationState *s;
>
> -    s = g_malloc0(sizeof(*s));
> +    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);
>
>       s->fd = monitor_get_fd(mon, fdname);
>       if (s->fd == -1) {
> @@ -75,20 +75,6 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,
>       s->get_error = fd_errno;
>       s->write = fd_write;
>       s->close = fd_close;
> -    s->cancel = migrate_fd_cancel;
> -    s->get_status = migrate_fd_get_status;
> -    s->release = migrate_fd_release;
> -
> -    s->blk = blk;
> -    s->shared = inc;
> -
> -    s->state = MIG_STATE_ACTIVE;
> -    s->mon = NULL;
> -    s->bandwidth_limit = bandwidth_limit;
> -
> -    if (!detach) {
> -        migrate_fd_monitor_suspend(s, mon);
> -    }
>
>       migrate_fd_connect(s);
>       return s;
> diff --git a/migration-tcp.c b/migration-tcp.c
> index d6feb23..db8cea2 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -89,21 +89,12 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,
>       if (parse_host_port(&addr, host_port)<  0)
>           return NULL;
>
> -    s = g_malloc0(sizeof(*s));
> +    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);
>
>       s->get_error = socket_errno;
>       s->write = socket_write;
>       s->close = tcp_close;
> -    s->cancel = migrate_fd_cancel;
> -    s->get_status = migrate_fd_get_status;
> -    s->release = migrate_fd_release;
>
> -    s->blk = blk;
> -    s->shared = inc;
> -
> -    s->state = MIG_STATE_ACTIVE;
> -    s->mon = NULL;
> -    s->bandwidth_limit = bandwidth_limit;
>       s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>       if (s->fd == -1) {
>           g_free(s);
> @@ -112,10 +103,6 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,
>
>       socket_set_nonblock(s->fd);
>
> -    if (!detach) {
> -        migrate_fd_monitor_suspend(s, mon);
> -    }
> -
>       do {
>           ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
>           if (ret == -1)
> diff --git a/migration-unix.c b/migration-unix.c
> index 3b9017b..74c6dde 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -88,21 +88,12 @@ MigrationState *unix_start_outgoing_migration(Monitor *mon,
>       addr.sun_family = AF_UNIX;
>       snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", path);
>
> -    s = g_malloc0(sizeof(*s));
> +    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);
>
>       s->get_error = unix_errno;
>       s->write = unix_write;
>       s->close = unix_close;
> -    s->cancel = migrate_fd_cancel;
> -    s->get_status = migrate_fd_get_status;
> -    s->release = migrate_fd_release;
>
> -    s->blk = blk;
> -    s->shared = inc;
> -
> -    s->state = MIG_STATE_ACTIVE;
> -    s->mon = NULL;
> -    s->bandwidth_limit = bandwidth_limit;
>       s->fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>       if (s->fd<  0) {
>           DPRINTF("Unable to open socket");
> @@ -125,10 +116,6 @@ MigrationState *unix_start_outgoing_migration(Monitor *mon,
>           goto err_after_open;
>       }
>
> -    if (!detach) {
> -        migrate_fd_monitor_suspend(s, mon);
> -    }
> -
>       if (ret>= 0)
>           migrate_fd_connect(s);
>
> diff --git a/migration.c b/migration.c
> index b27a322..990667e 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -263,7 +263,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
>
>   /* shared migration helpers */
>
> -void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
> +static void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
>   {
>       s->mon = mon;
>       if (monitor_suspend(mon) == 0) {
> @@ -404,12 +404,12 @@ void migrate_fd_put_ready(void *opaque)
>       }
>   }
>
> -int migrate_fd_get_status(MigrationState *s)
> +static int migrate_fd_get_status(MigrationState *s)
>   {
>       return s->state;
>   }
>
> -void migrate_fd_cancel(MigrationState *s)
> +static void migrate_fd_cancel(MigrationState *s)
>   {
>       if (s->state != MIG_STATE_ACTIVE)
>           return;
> @@ -423,7 +423,7 @@ void migrate_fd_cancel(MigrationState *s)
>       migrate_fd_cleanup(s);
>   }
>
> -void migrate_fd_release(MigrationState *s)
> +static void migrate_fd_release(MigrationState *s)
>   {
>
>       DPRINTF("releasing state\n");
> @@ -488,3 +488,24 @@ int get_migration_state(void)
>           return MIG_STATE_ERROR;
>       }
>   }
> +
> +MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
> +                                     int detach, int blk, int inc)


I think this would fit better if it were migrate_new() or migrate_open().

It definitely should be in the form noun_verb and not noun_verb_noun().

Regards,

Anthony Liguori

> +{
> +    MigrationState *s = g_malloc0(sizeof(*s));
> +
> +    s->cancel = migrate_fd_cancel;
> +    s->get_status = migrate_fd_get_status;
> +    s->release = migrate_fd_release;
> +    s->blk = blk;
> +    s->shared = inc;
> +    s->mon = NULL;
> +    s->bandwidth_limit = bandwidth_limit;
> +    s->state = MIG_STATE_ACTIVE;
> +
> +    if (!detach) {
> +        migrate_fd_monitor_suspend(s, mon);
> +    }
> +
> +    return s;
> +}
> diff --git a/migration.h b/migration.h
> index e24efed..32462f7 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -98,8 +98,6 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,
>   					    int blk,
>   					    int inc);
>
> -void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon);
> -
>   void migrate_fd_error(MigrationState *s);
>
>   int migrate_fd_cleanup(MigrationState *s);
> @@ -112,16 +110,13 @@ void migrate_fd_connect(MigrationState *s);
>
>   void migrate_fd_put_ready(void *opaque);
>
> -int migrate_fd_get_status(MigrationState *mig_state);
> -
> -void migrate_fd_cancel(MigrationState *mig_state);
> -
> -void migrate_fd_release(MigrationState *mig_state);
> -
>   void migrate_fd_wait_for_unfreeze(void *opaque);
>
>   int migrate_fd_close(void *opaque);
>
> +MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
> +                                     int detach, int blk, int inc);
> +
>   void add_migration_state_change_notifier(Notifier *notify);
>   void remove_migration_state_change_notifier(Notifier *notify);
>   int get_migration_state(void);