Re: [Qemu-devel] [PATCH 6/6] qapi: Convert migrate

[Wen Congyang]

At 02/11/2012 03:31 AM, Luiz Capitulino Wrote:
> The migrate command is one of those commands where HMP and QMP completely
> mix up together. This made the conversion to the QAPI (which separates the
> command into QMP and HMP parts) a bit difficult.
> 
> The first important change to be noticed is that this commit completes the
> removal of the Monitor object from migration code, started by the previous
> commit.
> 
> Another important and tricky change is about supporting the non-detached
> mode. That's, if the user doesn't pass '-d' the migrate command will lock
> the monitor and will only release it when migration is finished.
> 
> To support that in the new HMP command (hmp_migrate()), it was necessary
> to create a timer which runs every second and checks if the migration is
> still active. If it's, the timer callback will re-schedule itself to run
> one second in the future. If the migration has already finished, the
> monitor lock is relased and the user can use it normally.
> 
> All these changes should be transparent to the user.
> 
> Signed-off-by: Anthony Liguori <address@hidden>
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---
>  hmp-commands.hx  |    3 +-
>  hmp.c            |   51 +++++++++++++++++++++++++++++++++++++++++
>  hmp.h            |    1 +
>  migration-fd.c   |    2 +-
>  migration.c      |   66 
> ++++++++++++++----------------------------------------
>  migration.h      |    3 --
>  qapi-schema.json |   21 +++++++++++++++++
>  qmp-commands.hx  |    9 +------
>  savevm.c         |   13 +++++-----
>  sysemu.h         |    2 +-
>  10 files changed, 100 insertions(+), 71 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 573b823..10d3f1b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -792,8 +792,7 @@ ETEXI
>                     " full copy of disk\n\t\t\t -i for migration without "
>                     "shared storage with incremental copy of disk "
>                     "(base image shared between src and destination)",
> -        .user_print = monitor_user_noop,     
> -     .mhandler.cmd_new = do_migrate,
> +        .mhandler.cmd = hmp_migrate,
>      },
>  
>  
> diff --git a/hmp.c b/hmp.c
> index 8ff8c94..70bd574 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include "hmp.h"
> +#include "qemu-timer.h"
>  #include "qmp-commands.h"
>  
>  static void hmp_handle_error(Monitor *mon, Error **errp)
> @@ -851,3 +852,53 @@ void hmp_block_job_cancel(Monitor *mon, const QDict 
> *qdict)
>  
>      hmp_handle_error(mon, &error);
>  }
> +
> +typedef struct MigrationStatus
> +{
> +    QEMUTimer *timer;
> +    Monitor *mon;
> +} MigrationStatus;
> +
> +static void hmp_migrate_status_cb(void *opaque)
> +{
> +    MigrationStatus *status = opaque;
> +    MigrationInfo *info;
> +
> +    info = qmp_query_migrate(NULL);
> +    if (!info->has_status || strcmp(info->status, "active") == 0) {
> +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock) + 1000);
> +        goto out;
> +    }
> +
> +    monitor_resume(status->mon);
> +    qemu_del_timer(status->timer);
> +    g_free(status);
> +
> +out:
> +    qapi_free_MigrationInfo(info);
> +}
> +
> +void hmp_migrate(Monitor *mon, const QDict *qdict)
> +{
> +    int detach = qdict_get_try_bool(qdict, "detach", 0);
> +    int blk = qdict_get_try_bool(qdict, "blk", 0);
> +    int inc = qdict_get_try_bool(qdict, "inc", 0);
> +    const char *uri = qdict_get_str(qdict, "uri");
> +    Error *err = NULL;
> +
> +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> +    if (err) {
> +        monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
> +        error_free(err);
> +        return;
> +    }
> +
> +    if (!detach) {
> +        MigrationStatus *status = g_malloc0(sizeof(*status));
> +        status->timer = qemu_new_timer_ms(rt_clock, hmp_migrate_status_cb,
> +                                          status);
> +        status->mon = mon;
> +        monitor_suspend(mon);
> +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
> +    }
> +}
> diff --git a/hmp.h b/hmp.h
> index 18eecbd..989a613 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -58,5 +58,6 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict 
> *qdict);
>  void hmp_block_stream(Monitor *mon, const QDict *qdict);
>  void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
>  void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
> +void hmp_migrate(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/migration-fd.c b/migration-fd.c
> index 5a068c6..99f192d 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -75,7 +75,7 @@ static int fd_close(MigrationState *s)
>  
>  int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
>  {
> -    s->fd = monitor_get_fd(s->mon, fdname);
> +    s->fd = qemu_get_fd(fdname);
>      if (s->fd == -1) {
>          DPRINTF("fd_migration: invalid file descriptor identifier\n");
>          goto err_after_get_fd;
> diff --git a/migration.c b/migration.c
> index 5c5c94c..6f148f9 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -158,16 +158,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>  
>  /* shared migration helpers */
>  
> -static void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
> -{
> -    if (monitor_suspend(mon) == 0) {
> -        DPRINTF("suspending monitor\n");
> -    } else {
> -        monitor_printf(mon, "terminal does not allow synchronous "
> -                       "migration, continuing detached\n");
> -    }
> -}
> -
>  static int migrate_fd_cleanup(MigrationState *s)
>  {
>      int ret = 0;
> @@ -178,10 +168,6 @@ static int migrate_fd_cleanup(MigrationState *s)
>          DPRINTF("closing file\n");
>          ret = qemu_fclose(s->file);
>          s->file = NULL;
> -    } else {
> -        if (s->mon) {
> -            monitor_resume(s->mon);
> -        }
>      }
>  
>      if (s->fd != -1) {
> @@ -321,9 +307,6 @@ static int migrate_fd_close(void *opaque)
>  {
>      MigrationState *s = opaque;
>  
> -    if (s->mon) {
> -        monitor_resume(s->mon);
> -    }
>      qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>      return s->close(s);
>  }
> @@ -376,7 +359,7 @@ void migrate_fd_connect(MigrationState *s)
>      migrate_fd_put_ready(s);
>  }
>  
> -static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int 
> inc)
> +static MigrationState *migrate_init(int blk, int inc)
>  {
>      MigrationState *s = migrate_get_current();
>      int64_t bandwidth_limit = s->bandwidth_limit;
> @@ -386,18 +369,9 @@ static MigrationState *migrate_init(Monitor *mon, int 
> detach, int blk, int inc)
>      s->blk = blk;
>      s->shared = inc;
>  
> -    /* s->mon is used for two things:
> -       - pass fd in fd migration
> -       - suspend/resume monitor for not detached migration
> -    */
> -    s->mon = mon;
>      s->bandwidth_limit = bandwidth_limit;
>      s->state = MIG_STATE_SETUP;
>  
> -    if (!detach) {
> -        migrate_fd_monitor_suspend(s, mon);
> -    }
> -
>      return s;
>  }
>  
> @@ -413,32 +387,29 @@ void migrate_del_blocker(Error *reason)
>      migration_blockers = g_slist_remove(migration_blockers, reason);
>  }
>  
> -int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void qmp_migrate(const char *uri, bool has_blk, bool blk,
> +                 bool has_inc, bool inc, bool has_detach, bool detach,
> +                 Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
>      const char *p;
> -    int detach = qdict_get_try_bool(qdict, "detach", 0);
> -    int blk = qdict_get_try_bool(qdict, "blk", 0);
> -    int inc = qdict_get_try_bool(qdict, "inc", 0);
> -    const char *uri = qdict_get_str(qdict, "uri");
>      int ret;
>  
>      if (s->state == MIG_STATE_ACTIVE) {
> -        monitor_printf(mon, "migration already in progress\n");
> -        return -1;
> +        error_set(errp, QERR_MIGRATION_ACTIVE);
> +        return;
>      }
>  
> -    if (qemu_savevm_state_blocked(mon)) {
> -        return -1;
> +    if (qemu_savevm_state_blocked(errp)) {
> +        return;
>      }
>  
>      if (migration_blockers) {
> -        Error *err = migration_blockers->data;
> -        qerror_report_err(err);
> -        return -1;
> +        *errp = error_copy(migration_blockers->data);
> +        return;
>      }
>  
> -    s = migrate_init(mon, detach, blk, inc);

It seems that the variable detach is not used now.
Does it mean we always run migration at the background?

Thanks
Wen Congyang
> +    s = migrate_init(blk, inc);
>  
>      if (strstart(uri, "tcp:", &p)) {
>          ret = tcp_start_outgoing_migration(s, p);
> @@ -451,21 +422,18 @@ int do_migrate(Monitor *mon, const QDict *qdict, 
> QObject **ret_data)
>          ret = fd_start_outgoing_migration(s, p);
>  #endif
>      } else {
> -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> -        ret  = -EINVAL;
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid 
> migration protocol");
> +        return;
>      }
>  
>      if (ret < 0) {
> -        monitor_printf(mon, "migration failed: %s\n", strerror(-ret));
> -        return ret;
> -    }
> -
> -    if (detach) {
> -        s->mon = NULL;
> +        DPRINTF("migration failed: %s\n", strerror(-ret));
> +        /* FIXME: we should return meaningful errors */
> +        error_set(errp, QERR_UNDEFINED_ERROR);
> +        return;
>      }
>  
>      notifier_list_notify(&migration_state_notifiers, s);
> -    return 0;
>  }
>  
>  void qmp_migrate_cancel(Error **errp)
> diff --git a/migration.h b/migration.h
> index 0e44197..691b367 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -26,7 +26,6 @@ struct MigrationState
>      int64_t bandwidth_limit;
>      QEMUFile *file;
>      int fd;
> -    Monitor *mon;
>      int state;
>      int (*get_error)(MigrationState *s);
>      int (*close)(MigrationState *s);
> @@ -40,8 +39,6 @@ void process_incoming_migration(QEMUFile *f);
>  
>  int qemu_start_incoming_migration(const char *uri);
>  
> -int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
> -
>  uint64_t migrate_max_downtime(void);
>  
>  void do_info_migrate_print(Monitor *mon, const QObject *data);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d02ee86..0cdc3ce 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1582,3 +1582,24 @@
>  { 'command': 'qom-list-types',
>    'data': { '*implements': 'str', '*abstract': 'bool' },
>    'returns': [ 'ObjectTypeInfo' ] }
> +
> +##
> +# @migrate
> +#
> +# Migrates the current running guest to another Virtual Machine.
> +#
> +# @uri: the Uniform Resource Identifier of the destination VM
> +#
> +# @blk: #optional do block migration (full disk copy)
> +#
> +# @inc: #optional incremental disk copy migration
> +#
> +# @detach: this argument exists only for compatibility reasons and should not
> +#          be used.
> +#
> +# Returns: nothing on success
> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'migrate',
> +  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } 
> }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b5e2ab8..84b348a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -425,14 +425,7 @@ EQMP
>      {
>          .name       = "migrate",
>          .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
> -        .params     = "[-d] [-b] [-i] uri",
> -        .help       = "migrate to URI (using -d to not wait for completion)"
> -                   "\n\t\t\t -b for migration without shared storage with"
> -                   " full copy of disk\n\t\t\t -i for migration without "
> -                   "shared storage with incremental copy of disk "
> -                   "(base image shared between src and destination)",
> -        .user_print = monitor_user_noop,     
> -     .mhandler.cmd_new = do_migrate,
> +        .mhandler.cmd_new = qmp_marshal_input_migrate,
>      },
>  
>  SQMP
> diff --git a/savevm.c b/savevm.c
> index 70f5c4f..5fdc3e1 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1540,14 +1540,13 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry 
> *se)
>  #define QEMU_VM_SECTION_FULL         0x04
>  #define QEMU_VM_SUBSECTION           0x05
>  
> -bool qemu_savevm_state_blocked(Monitor *mon)
> +bool qemu_savevm_state_blocked(Error **errp)
>  {
>      SaveStateEntry *se;
>  
>      QTAILQ_FOREACH(se, &savevm_handlers, entry) {
>          if (se->no_migrate) {
> -            monitor_printf(mon, "state blocked by non-migratable device 
> '%s'\n",
> -                           se->idstr);
> +            error_set(errp, QERR_MIGRATION_NOT_SUPPORTED, se->idstr);
>              return true;
>          }
>      }
> @@ -1698,11 +1697,11 @@ void qemu_savevm_state_cancel(QEMUFile *f)
>      }
>  }
>  
> -static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
> +static int qemu_savevm_state(QEMUFile *f)
>  {
>      int ret;
>  
> -    if (qemu_savevm_state_blocked(mon)) {
> +    if (qemu_savevm_state_blocked(NULL)) {
>          ret = -EINVAL;
>          goto out;
>      }
> @@ -1836,7 +1835,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      unsigned int v;
>      int ret;
>  
> -    if (qemu_savevm_state_blocked(default_mon)) {
> +    if (qemu_savevm_state_blocked(NULL)) {
>          return -EINVAL;
>      }
>  
> @@ -2080,7 +2079,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "Could not open VM state file\n");
>          goto the_end;
>      }
> -    ret = qemu_savevm_state(mon, f);
> +    ret = qemu_savevm_state(f);
>      vm_state_size = qemu_ftell(f);
>      qemu_fclose(f);
>      if (ret < 0) {
> diff --git a/sysemu.h b/sysemu.h
> index 3a0f683..18c07ca 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -65,7 +65,7 @@ void do_info_snapshots(Monitor *mon);
>  
>  void qemu_announce_self(void);
>  
> -bool qemu_savevm_state_blocked(Monitor *mon);
> +bool qemu_savevm_state_blocked(Error **errp);
>  int qemu_savevm_state_begin(QEMUFile *f, int blk_enable, int shared);
>  int qemu_savevm_state_iterate(QEMUFile *f);
>  int qemu_savevm_state_complete(QEMUFile *f);