qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alon Levy
Subject: Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate
Date: Mon, 12 Mar 2012 15:49:27 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

On Mon, Mar 12, 2012 at 10:21:20AM -0300, Luiz Capitulino wrote:
> On Sat, 10 Mar 2012 00:51:29 +0200
> Alon Levy <address@hidden> wrote:
> 
> > On Fri, Mar 09, 2012 at 03:13:06PM -0300, 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 is 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            |   56 +++++++++++++++++++++++++++++++++++++++++++++
> > >  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, 105 insertions(+), 71 deletions(-)
> > > 
> > > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > > index ed88877..a86f53f 100644
> > > --- a/hmp-commands.hx
> > > +++ b/hmp-commands.hx
> > > @@ -806,8 +806,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 3a54455..0f62b3f 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)
> > > @@ -856,3 +857,58 @@ 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);
> > > +    } else {
> > > +        monitor_resume(status->mon);
> > > +        qemu_del_timer(status->timer);
> > > +        g_free(status);
> > > +    }
> > > +
> > > +    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;
> > > +
> > > +        if (monitor_suspend(mon) < 0) {
> > 
> > So using monitor_suspend is ok? is that a possible solution for the
> > screendump issue? I could pass the Monitor through the vga callback. Tbh
> > I think it's ugly, but at least it doesn't introduce any new command.
> 
> This is only valid for HMP.
> 

Yes, I'm thinking at this point for qmp to wait for the output file to
be written via repeated poll / inotify.

> > 
> > > +            monitor_printf(mon, "terminal does not allow synchronous "
> > > +                           "migration, continuing detached\n");
> > > +            return;
> > > +        }
> > > +
> > > +        status = g_malloc0(sizeof(*status));
> > > +        status->mon = mon;
> > > +        status->timer = qemu_new_timer_ms(rt_clock, 
> > > hmp_migrate_status_cb,
> > > +                                          status);
> > > +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
> > > +    }
> > > +}
> > > diff --git a/hmp.h b/hmp.h
> > > index 5409464..8807853 100644
> > > --- a/hmp.h
> > > +++ b/hmp.h
> > > @@ -59,5 +59,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..50138ed 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 = monitor_get_fd(cur_mon, 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 b21b2df..8c119ba 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);
> > > +    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 5f293c4..c0780ab 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -1631,3 +1631,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 0c9bfac..f328bc1 100644
> > > --- a/qmp-commands.hx
> > > +++ b/qmp-commands.hx
> > > @@ -446,14 +446,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 29b0e96..bc2c788 100644
> > > --- a/sysemu.h
> > > +++ b/sysemu.h
> > > @@ -76,7 +76,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);
> > > -- 
> > > 1.7.9.2.384.g4a92a
> > > 
> > > 
> > 
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]