Re: [Qemu-devel] [PATCH COLO-Frame v9 08/32] COLO/migration: establish a new communication path from destination to source

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
> On 2015/10/19 17:54, Dr. David Alan Gilbert wrote:
> >* zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
> >>Add a new member 'to_src_file' to MigrationIncomingState and a
> >>new member 'from_dst_file' to MigrationState.
> >>They will be used for returning messages from destination to source.
> >>It will also be used by post-copy migration.
> >>
> >>Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> >>Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> >>Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>---
> >>  include/migration/migration.h |  3 ++-
> >>  migration/colo.c              | 43 +++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 45 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/include/migration/migration.h b/include/migration/migration.h
> >>index 6488e03..0c94103 100644
> >>--- a/include/migration/migration.h
> >>+++ b/include/migration/migration.h
> >>@@ -50,7 +50,7 @@ typedef QLIST_HEAD(, LoadStateEntry) LoadStateEntry_Head;
> >>  /* State for the incoming migration */
> >>  struct MigrationIncomingState {
> >>      QEMUFile *from_src_file;
> >>-
> >>+    QEMUFile *to_src_file;
> >>      int state;
> >>
> >>      bool have_colo_incoming_thread;
> >>@@ -74,6 +74,7 @@ struct MigrationState
> >>      QemuThread thread;
> >>      QEMUBH *cleanup_bh;
> >>      QEMUFile *to_dst_file;
> >>+    QEMUFile  *from_dst_file;
> >>      int parameters[MIGRATION_PARAMETER_MAX];
> >>
> >>      int state;
> >>diff --git a/migration/colo.c b/migration/colo.c
> >>index a341eee..5f4fb20 100644
> >>--- a/migration/colo.c
> >>+++ b/migration/colo.c
> >>@@ -39,6 +39,20 @@ bool migration_incoming_in_colo_state(void)
> >>  static void *colo_thread(void *opaque)
> >>  {
> >>      MigrationState *s = opaque;
> >>+    int fd, ret = 0;
> >>+
> >>+    /* Dup the fd of to_dst_file */
> >>+    fd = dup(qemu_get_fd(s->to_dst_file));
> >>+    if (fd == -1) {
> >>+        ret = -errno;
> >>+        goto out;
> >>+    }
> >>+    s->from_dst_file = qemu_fopen_socket(fd, "rb");
> >
> >In my postcopy code I add the return-path opening as a new
> >method on QEMUFile, that way if we get a return path working
> >on another transport (RDMA which I hope to do) then it
> >works;  have a look at 'Return path: Open a return path on QEMUFile for sockets'
> >
> 
> I have looked at it. That's a good solution, we use the same fd for return path, and
> i don't have to call qemu_file_shutdown two times in failover process.
> 
> >>+    if (!s->from_dst_file) {
> >>+        ret = -EINVAL;
> >>+        error_report("Open QEMUFile failed!");
> >
> >In errors, try to give detail of where a problem was;
> >e.g. 'colo_thread: Open QEMUFile from_dst failed'.
> >
> 
> OK. I will fix it in next version.
> 
> >>+        goto out;
> >>+    }
> >>
> >>      qemu_mutex_lock_iothread();
> >>      vm_start();
> >>@@ -47,9 +61,17 @@ static void *colo_thread(void *opaque)
> >>
> >>      /*TODO: COLO checkpoint savevm loop*/
> >>
> >>+out:
> >>+    if (ret < 0) {
> >>+        error_report("Detect some error: %s", strerror(-ret));
> >>+    }
> >
> >Again, best to say where the error happened.
> >
> 
> Hmm, it is a little difficult to say where exactly this error happened here,
> what i can do is to error out in the place where the error happened.
> Here is only a summary for the error.

OK, just add the function name then, e.g.
   error_report("%s: %s", __func__, strerror(-ret));

  it just helps when you're looking at a log file, to understand where it came
from; especially if the person who sent you the log file didn't tell you much :-)

Dave
> 
> >
> >>      migrate_set_state(&s->state, MIGRATION_STATUS_COLO,
> >>                        MIGRATION_STATUS_COMPLETED);
> >>
> >>+    if (s->from_dst_file) {
> >>+        qemu_fclose(s->from_dst_file);
> >>+    }
> >>+
> >>      qemu_mutex_lock_iothread();
> >>      qemu_bh_schedule(s->cleanup_bh);
> >>      qemu_mutex_unlock_iothread();
> >>@@ -86,12 +108,33 @@ void colo_init_checkpointer(MigrationState *s)
> >>  void *colo_process_incoming_thread(void *opaque)
> >>  {
> >>      MigrationIncomingState *mis = opaque;
> >>+    int fd, ret = 0;
> >>
> >>      migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> >>                        MIGRATION_STATUS_COLO);
> >>
> >>+    fd = dup(qemu_get_fd(mis->from_src_file));
> >>+    if (fd < 0) {
> >>+        ret = -errno;
> >>+        goto out;
> >>+    }
> >>+    mis->to_src_file = qemu_fopen_socket(fd, "wb");
> >>+    if (!mis->to_src_file) {
> >>+        ret = -EINVAL;
> >>+        error_report("Can't open incoming channel!");
> >>+        goto out;
> >>+    }
> >
> >Same as above.
> 
> OK. Will fix it.
> 
> Thanks,
> zhanghailiang
> >
> >>      /* TODO: COLO checkpoint restore loop */
> >>
> >>+out:
> >>+    if (ret < 0) {
> >
> >>+        error_report("colo incoming thread will exit, detect error: %s",
> >>+                     strerror(-ret));
> >>+    }
> >>+
> >>+    if (mis->to_src_file) {
> >>+        qemu_fclose(mis->to_src_file);
> >>+    }
> >>      migration_incoming_exit_colo();
> >>
> >>      return NULL;
> >>--
> >>1.8.3.1
> >>
> >>
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >.
> >
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK