[Qemu-devel] [PATCH 0/2] Misc migration fixes

[Qemu-devel] [PATCH 0/2] Misc migration fixes

[Juan Quintela]

Hi

This are independent of all my series, so send then here.

- check_migratable needs to now about objects, device class, etc, that
  migration code don't care.  So move it to qdev.c.

- to_dst_file is set to NULL before this call, just remove it.  Long
  term idea is that nothing outside migration.c should now about
  members of MigrationState.

Please, review.

Juan Quintela (2):
  migration: Move check_migratable() into qdev.c
  migration: to_dst_file at that point is NULL

 hw/core/qdev.c                | 15 ++++++++++++++-
 include/migration/migration.h |  3 ---
 include/migration/vmstate.h   |  2 ++
 migration/migration.c         | 15 ---------------
 migration/savevm.c            | 10 ++++++++++
 migration/socket.c            |  1 -
 migration/tls.c               |  1 -
 stubs/vmstate.c               |  5 ++---
 8 files changed, 28 insertions(+), 24 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/2] migration: Move check_migratable() into qdev.c

[Juan Quintela]

The function is only used once, and nothing else in migration knows
about objects.  Create the function vmstate_device_is_migratable() in
savem.c that really do the bit that is related with migration.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/core/qdev.c                | 15 ++++++++++++++-
 include/migration/migration.h |  3 ---
 include/migration/vmstate.h   |  2 ++
 migration/migration.c         | 15 ---------------
 migration/savevm.c            | 10 ++++++++++
 stubs/vmstate.c               |  5 ++---
 6 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 02b632f..17ff638 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -37,7 +37,7 @@
 #include "hw/boards.h"
 #include "hw/sysbus.h"
 #include "qapi-event.h"
-#include "migration/migration.h"
+#include "migration/vmstate.h"
 
 bool qdev_hotplug = false;
 static bool qdev_hot_added = false;
@@ -861,6 +861,19 @@ static bool device_get_realized(Object *obj, Error **errp)
     return dev->realized;
 }
 
+static int check_migratable(Object *obj, Error **err)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(obj);
+    if (!vmstate_device_is_migratable(dc->vmsd)) {
+        error_setg(err, "Device %s is not migratable, but "
+                   "--only-migratable was specified",
+                   object_get_typename(obj));
+        return -1;
+    }
+
+    return 0;
+}
+
 static void device_set_realized(Object *obj, bool value, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index ba1a16c..dfeca38 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -22,7 +22,6 @@
 #include "qapi-types.h"
 #include "exec/cpu-common.h"
 #include "qemu/coroutine_int.h"
-#include "qom/object.h"
 
 #define QEMU_VM_FILE_MAGIC           0x5145564d
 #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
@@ -292,8 +291,6 @@ int migrate_add_blocker(Error *reason, Error **errp);
  */
 void migrate_del_blocker(Error *reason);
 
-int check_migratable(Object *obj, Error **err);
-
 bool migrate_release_ram(void);
 bool migrate_postcopy_ram(void);
 bool migrate_zero_blocks(void);
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index dad3984..9452dec 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1049,4 +1049,6 @@ int64_t self_announce_delay(int round)
 
 void dump_vmstate_json_to_file(FILE *out_fp);
 
+bool vmstate_device_is_migratable(const VMStateDescription *vmsd);
+
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 353f272..5447cab 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1158,21 +1158,6 @@ void migrate_del_blocker(Error *reason)
     migration_blockers = g_slist_remove(migration_blockers, reason);
 }
 
-int check_migratable(Object *obj, Error **err)
-{
-    DeviceClass *dc = DEVICE_GET_CLASS(obj);
-    if (only_migratable && dc->vmsd) {
-        if (dc->vmsd->unmigratable) {
-            error_setg(err, "Device %s is not migratable, but "
-                       "--only-migratable was specified",
-                       object_get_typename(obj));
-            return -1;
-        }
-    }
-
-    return 0;
-}
-
 void qmp_migrate_incoming(const char *uri, Error **errp)
 {
     Error *local_err = NULL;
diff --git a/migration/savevm.c b/migration/savevm.c
index 03ae1bd..7421a67 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2480,3 +2480,13 @@ void vmstate_register_ram_global(MemoryRegion *mr)
 {
     vmstate_register_ram(mr, NULL);
 }
+
+bool vmstate_device_is_migratable(const VMStateDescription *vmsd)
+{
+    if (only_migratable && vmsd) {
+        if (vmsd->unmigratable) {
+            return false;
+        }
+    }
+    return true;
+}
diff --git a/stubs/vmstate.c b/stubs/vmstate.c
index 6d52f29..5af824b 100644
--- a/stubs/vmstate.c
+++ b/stubs/vmstate.c
@@ -1,7 +1,6 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "migration/vmstate.h"
-#include "migration/migration.h"
 
 const VMStateDescription vmstate_dummy = {};
 
@@ -21,7 +20,7 @@ void vmstate_unregister(DeviceState *dev,
 {
 }
 
-int check_migratable(Object *obj, Error **err)
+bool vmstate_device_is_migratable(const VMStateDescription *vmsd)
 {
-    return 0;
+    return true;
 }
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/2] migration: Move check_migratable() into qdev.c

[Laurent Vivier]

On 25/04/2017 12:17, Juan Quintela wrote:
> The function is only used once, and nothing else in migration knows
> about objects.  Create the function vmstate_device_is_migratable() in
> savem.c that really do the bit that is related with migration.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  hw/core/qdev.c                | 15 ++++++++++++++-
>  include/migration/migration.h |  3 ---
>  include/migration/vmstate.h   |  2 ++
>  migration/migration.c         | 15 ---------------
>  migration/savevm.c            | 10 ++++++++++
>  stubs/vmstate.c               |  5 ++---
>  6 files changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 02b632f..17ff638 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -37,7 +37,7 @@
>  #include "hw/boards.h"
>  #include "hw/sysbus.h"
>  #include "qapi-event.h"
> -#include "migration/migration.h"
> +#include "migration/vmstate.h"
>  
>  bool qdev_hotplug = false;
>  static bool qdev_hot_added = false;
> @@ -861,6 +861,19 @@ static bool device_get_realized(Object *obj, Error **errp)
>      return dev->realized;
>  }
>  
> +static int check_migratable(Object *obj, Error **err)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +    if (!vmstate_device_is_migratable(dc->vmsd)) {
> +        error_setg(err, "Device %s is not migratable, but "
> +                   "--only-migratable was specified",
> +                   object_get_typename(obj));
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  static void device_set_realized(Object *obj, bool value, Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index ba1a16c..dfeca38 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -22,7 +22,6 @@
>  #include "qapi-types.h"
>  #include "exec/cpu-common.h"
>  #include "qemu/coroutine_int.h"
> -#include "qom/object.h"
>  
>  #define QEMU_VM_FILE_MAGIC           0x5145564d
>  #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
> @@ -292,8 +291,6 @@ int migrate_add_blocker(Error *reason, Error **errp);
>   */
>  void migrate_del_blocker(Error *reason);
>  
> -int check_migratable(Object *obj, Error **err);
> -
>  bool migrate_release_ram(void);
>  bool migrate_postcopy_ram(void);
>  bool migrate_zero_blocks(void);
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index dad3984..9452dec 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1049,4 +1049,6 @@ int64_t self_announce_delay(int round)
>  
>  void dump_vmstate_json_to_file(FILE *out_fp);
>  
> +bool vmstate_device_is_migratable(const VMStateDescription *vmsd);
> +
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 353f272..5447cab 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1158,21 +1158,6 @@ void migrate_del_blocker(Error *reason)
>      migration_blockers = g_slist_remove(migration_blockers, reason);
>  }
>  
> -int check_migratable(Object *obj, Error **err)
> -{
> -    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> -    if (only_migratable && dc->vmsd) {
> -        if (dc->vmsd->unmigratable) {
> -            error_setg(err, "Device %s is not migratable, but "
> -                       "--only-migratable was specified",
> -                       object_get_typename(obj));
> -            return -1;
> -        }
> -    }
> -
> -    return 0;
> -}
> -
>  void qmp_migrate_incoming(const char *uri, Error **errp)
>  {
>      Error *local_err = NULL;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 03ae1bd..7421a67 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2480,3 +2480,13 @@ void vmstate_register_ram_global(MemoryRegion *mr)
>  {
>      vmstate_register_ram(mr, NULL);
>  }
> +
> +bool vmstate_device_is_migratable(const VMStateDescription *vmsd)
> +{
> +    if (only_migratable && vmsd) {
> +        if (vmsd->unmigratable) {
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> diff --git a/stubs/vmstate.c b/stubs/vmstate.c
> index 6d52f29..5af824b 100644
> --- a/stubs/vmstate.c
> +++ b/stubs/vmstate.c
> @@ -1,7 +1,6 @@
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "migration/vmstate.h"
> -#include "migration/migration.h"
>  
>  const VMStateDescription vmstate_dummy = {};
>  
> @@ -21,7 +20,7 @@ void vmstate_unregister(DeviceState *dev,
>  {
>  }
>  
> -int check_migratable(Object *obj, Error **err)
> +bool vmstate_device_is_migratable(const VMStateDescription *vmsd)
>  {
> -    return 0;
> +    return true;
>  }
> 

Re: [Qemu-devel] [PATCH 1/2] migration: Move check_migratable() into qdev.c

[Peter Xu]

On Tue, Apr 25, 2017 at 12:17:57PM +0200, Juan Quintela wrote:
> The function is only used once, and nothing else in migration knows
> about objects.  Create the function vmstate_device_is_migratable() in
> savem.c that really do the bit that is related with migration.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/core/qdev.c                | 15 ++++++++++++++-
>  include/migration/migration.h |  3 ---
>  include/migration/vmstate.h   |  2 ++
>  migration/migration.c         | 15 ---------------
>  migration/savevm.c            | 10 ++++++++++
>  stubs/vmstate.c               |  5 ++---
>  6 files changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 02b632f..17ff638 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -37,7 +37,7 @@
>  #include "hw/boards.h"
>  #include "hw/sysbus.h"
>  #include "qapi-event.h"
> -#include "migration/migration.h"
> +#include "migration/vmstate.h"
>  
>  bool qdev_hotplug = false;
>  static bool qdev_hot_added = false;
> @@ -861,6 +861,19 @@ static bool device_get_realized(Object *obj, Error **errp)
>      return dev->realized;
>  }
>  
> +static int check_migratable(Object *obj, Error **err)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +    if (!vmstate_device_is_migratable(dc->vmsd)) {
> +        error_setg(err, "Device %s is not migratable, but "
> +                   "--only-migratable was specified",
> +                   object_get_typename(obj));
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  static void device_set_realized(Object *obj, bool value, Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index ba1a16c..dfeca38 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -22,7 +22,6 @@
>  #include "qapi-types.h"
>  #include "exec/cpu-common.h"
>  #include "qemu/coroutine_int.h"
> -#include "qom/object.h"
>  
>  #define QEMU_VM_FILE_MAGIC           0x5145564d
>  #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
> @@ -292,8 +291,6 @@ int migrate_add_blocker(Error *reason, Error **errp);
>   */
>  void migrate_del_blocker(Error *reason);
>  
> -int check_migratable(Object *obj, Error **err);
> -
>  bool migrate_release_ram(void);
>  bool migrate_postcopy_ram(void);
>  bool migrate_zero_blocks(void);
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index dad3984..9452dec 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1049,4 +1049,6 @@ int64_t self_announce_delay(int round)
>  
>  void dump_vmstate_json_to_file(FILE *out_fp);
>  
> +bool vmstate_device_is_migratable(const VMStateDescription *vmsd);
> +
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 353f272..5447cab 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1158,21 +1158,6 @@ void migrate_del_blocker(Error *reason)
>      migration_blockers = g_slist_remove(migration_blockers, reason);
>  }
>  
> -int check_migratable(Object *obj, Error **err)
> -{
> -    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> -    if (only_migratable && dc->vmsd) {
> -        if (dc->vmsd->unmigratable) {
> -            error_setg(err, "Device %s is not migratable, but "
> -                       "--only-migratable was specified",
> -                       object_get_typename(obj));
> -            return -1;
> -        }
> -    }
> -
> -    return 0;
> -}
> -
>  void qmp_migrate_incoming(const char *uri, Error **errp)
>  {
>      Error *local_err = NULL;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 03ae1bd..7421a67 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2480,3 +2480,13 @@ void vmstate_register_ram_global(MemoryRegion *mr)
>  {
>      vmstate_register_ram(mr, NULL);
>  }
> +
> +bool vmstate_device_is_migratable(const VMStateDescription *vmsd)
> +{
> +    if (only_migratable && vmsd) {
> +        if (vmsd->unmigratable) {
> +            return false;
> +        }
> +    }
> +    return true;
> +}

Here the name vmstate_device_is_migratable() let me think of "return
true if the device can migrate, false otherwise". However what it
actually does is mixed with "--only-migratable" parameter, say, when
without that parameter, all device will be getting "true" here, even
unmigratable devices...

Not sure whether it'll be nicer we do this:

bool vmstate_device_is_migratable(const VMStateDescription *vmsd)
{
    return !(vmsd & vmsd->unmigratable);
}

Then we can define check_migratable() as:

static int check_migratable(Object *obj, Error **err)
{
    DeviceClass *dc = DEVICE_GET_CLASS(obj);

    /* no check needed if --only-migratable not specified */
    if (!only_migratable) {
       return true;
    }

    if (!vmstate_device_is_migratable(dc->vmsd)) {
        error_setg(err, "Device %s is not migratable, but "
                   "--only-migratable was specified",
                   object_get_typename(obj));
        return -1;
    }

    return 0;
}

Thanks,

> diff --git a/stubs/vmstate.c b/stubs/vmstate.c
> index 6d52f29..5af824b 100644
> --- a/stubs/vmstate.c
> +++ b/stubs/vmstate.c
> @@ -1,7 +1,6 @@
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "migration/vmstate.h"
> -#include "migration/migration.h"
>  
>  const VMStateDescription vmstate_dummy = {};
>  
> @@ -21,7 +20,7 @@ void vmstate_unregister(DeviceState *dev,
>  {
>  }
>  
> -int check_migratable(Object *obj, Error **err)
> +bool vmstate_device_is_migratable(const VMStateDescription *vmsd)
>  {
> -    return 0;
> +    return true;
>  }
> -- 
> 2.9.3
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 1/2] migration: Move check_migratable() into qdev.c

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, Apr 25, 2017 at 12:17:57PM +0200, Juan Quintela wrote:
>> The function is only used once, and nothing else in migration knows
>> about objects.  Create the function vmstate_device_is_migratable() in
>> savem.c that really do the bit that is related with migration.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>

>
> Here the name vmstate_device_is_migratable() let me think of "return
> true if the device can migrate, false otherwise". However what it
> actually does is mixed with "--only-migratable" parameter, say, when
> without that parameter, all device will be getting "true" here, even
> unmigratable devices...
>
> Not sure whether it'll be nicer we do this:
>
> bool vmstate_device_is_migratable(const VMStateDescription *vmsd)
> {
>     return !(vmsd & vmsd->unmigratable);
> }
>
> Then we can define check_migratable() as:
>
> static int check_migratable(Object *obj, Error **err)
> {
>     DeviceClass *dc = DEVICE_GET_CLASS(obj);
>
>     /* no check needed if --only-migratable not specified */
>     if (!only_migratable) {
>        return true;
>     }
>
>     if (!vmstate_device_is_migratable(dc->vmsd)) {
>         error_setg(err, "Device %s is not migratable, but "
>                    "--only-migratable was specified",
>                    object_get_typename(obj));
>         return -1;
>     }
>
>     return 0;
> }
>
> Thanks,

I see your point.  We have to teach things about migrate to qdev.c or
things about objects to migration.

Will change.

Thanks, Juan.

[Qemu-devel] [PATCH 2/2] migration: to_dst_file at that point is NULL

[Juan Quintela]

We have just arrived as:

migration.c: qemu_migrate()
  ....
  s = migrate_init() <- puts it to NULL
  ....
  {tcp,unix}_start_outgoing_migration ->
     socket_outgoing_migration
        migration_channel_connect()
	   sets to_dst_file

if tls is enabled, we do another round through
migrate_channel_tls_connect(), but we only set it up if there is no
error.  So we don't need the assignation.  I am removing it to remove
in the follwing patches the knowledge about MigrationState in that two
files.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/socket.c | 1 -
 migration/tls.c    | 1 -
 2 files changed, 2 deletions(-)

diff --git a/migration/socket.c b/migration/socket.c
index 13966f1..dc88812 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task,
 
     if (qio_task_propagate_error(task, &err)) {
         trace_migration_socket_outgoing_error(error_get_pretty(err));
-        data->s->to_dst_file = NULL;
         migrate_fd_error(data->s, err);
         error_free(err);
     } else {
diff --git a/migration/tls.c b/migration/tls.c
index 45bec44..a33ecb7 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task,
 
     if (qio_task_propagate_error(task, &err)) {
         trace_migration_tls_outgoing_handshake_error(error_get_pretty(err));
-        s->to_dst_file = NULL;
         migrate_fd_error(s, err);
         error_free(err);
     } else {
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/2] migration: to_dst_file at that point is NULL

[Laurent Vivier]

On 25/04/2017 12:17, Juan Quintela wrote:
> We have just arrived as:
> 
> migration.c: qemu_migrate()
>   ....
>   s = migrate_init() <- puts it to NULL
>   ....
>   {tcp,unix}_start_outgoing_migration ->
>      socket_outgoing_migration
>         migration_channel_connect()
> 	   sets to_dst_file
> 
> if tls is enabled, we do another round through
> migrate_channel_tls_connect(), but we only set it up if there is no
> error.  So we don't need the assignation.  I am removing it to remove
> in the follwing patches the knowledge about MigrationState in that two
> files.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/socket.c | 1 -
>  migration/tls.c    | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/migration/socket.c b/migration/socket.c
> index 13966f1..dc88812 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task,
>  
>      if (qio_task_propagate_error(task, &err)) {
>          trace_migration_socket_outgoing_error(error_get_pretty(err));
> -        data->s->to_dst_file = NULL;
>          migrate_fd_error(data->s, err);
>          error_free(err);
>      } else {
> diff --git a/migration/tls.c b/migration/tls.c
> index 45bec44..a33ecb7 100644
> --- a/migration/tls.c
> +++ b/migration/tls.c
> @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task,
>  
>      if (qio_task_propagate_error(task, &err)) {
>          trace_migration_tls_outgoing_handshake_error(error_get_pretty(err));
> -        s->to_dst_file = NULL;
>          migrate_fd_error(s, err);
>          error_free(err);
>      } else {
> 

In migrate_fd_error(), we have "assert(s->to_dst_file == NULL);", so you
break the function with this change.

Laurent

Re: [Qemu-devel] [PATCH 2/2] migration: to_dst_file at that point is NULL

[Juan Quintela]

Laurent Vivier <lvivier@redhat.com> wrote:
> On 25/04/2017 12:17, Juan Quintela wrote:
>> We have just arrived as:
>> 
>> migration.c: qemu_migrate()
>>   ....
>>   s = migrate_init() <- puts it to NULL
>>   ....
>>   {tcp,unix}_start_outgoing_migration ->
>>      socket_outgoing_migration
>>         migration_channel_connect()
>> 	   sets to_dst_file
>> 
>> if tls is enabled, we do another round through
>> migrate_channel_tls_connect(), but we only set it up if there is no
>> error.  So we don't need the assignation.  I am removing it to remove
>> in the follwing patches the knowledge about MigrationState in that two
>> files.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/socket.c | 1 -
>>  migration/tls.c    | 1 -
>>  2 files changed, 2 deletions(-)
>> 
>> diff --git a/migration/socket.c b/migration/socket.c
>> index 13966f1..dc88812 100644
>> --- a/migration/socket.c
>> +++ b/migration/socket.c
>> @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task,
>>  
>>      if (qio_task_propagate_error(task, &err)) {
>>          trace_migration_socket_outgoing_error(error_get_pretty(err));
>> -        data->s->to_dst_file = NULL;
>>          migrate_fd_error(data->s, err);
>>          error_free(err);
>>      } else {
>> diff --git a/migration/tls.c b/migration/tls.c
>> index 45bec44..a33ecb7 100644
>> --- a/migration/tls.c
>> +++ b/migration/tls.c
>> @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task,
>>  
>>      if (qio_task_propagate_error(task, &err)) {
>>          trace_migration_tls_outgoing_handshake_error(error_get_pretty(err));
>> -        s->to_dst_file = NULL;
>>          migrate_fd_error(s, err);
>>          error_free(err);
>>      } else {
>> 
>
> In migrate_fd_error(), we have "assert(s->to_dst_file == NULL);", so you
> break the function with this change.

I missundertood something, or everyway to arrive here, to_dst_file is
always NULL.  See the comment of the file, the only distintion if we go
through tls is that we do "yet" another round through the init
functions, but it is still NULL.  At least I can't see how this can be
NULL at this point.

Forget about tls for now, centrate in the normal socket.c case:

we are inside socket_outgoing_migration()
    nothing here touch any componetes of data

so go to the caller:

socket_start_outgoing_migration().

It init all data values to NULL/0 (g_new0).

we call qio_channel_set_name() -> no "data" here.
qio_channel_socket_connect_async()

It touches "neworking" things here, but nothing related to data, the
first function that we call when we receive a connection is
socket_outgoing_migration(), so we are good.


Back to the tls case:

migration_tls_outgoing_handshake () -> nothing touch "s" until we call
migrate_fd_error().

what calls migration_tls_outgoing_handshake?
migration_tls_outgoing_connect().

But that only calls it using qio_channel_tls_handshake().  Notice that
they pass us here MigrationState, so, who calls this function?

migration_channel_connect(), this is the function that calls tls, and
tls ends calling this function without the tls stuff.  So, at the point
when we setup s->to_dst_file = f here, it is the 1st time that we have
set that field, too late to affect the migrate_fd_error() that we were
talking about, no?

Later, Juan.

Re: [Qemu-devel] [PATCH 2/2] migration: to_dst_file at that point is NULL

[Laurent Vivier]

On 25/04/2017 14:59, Juan Quintela wrote:
> Laurent Vivier <lvivier@redhat.com> wrote:
>> On 25/04/2017 12:17, Juan Quintela wrote:
>>> We have just arrived as:
>>>
>>> migration.c: qemu_migrate()
>>>   ....
>>>   s = migrate_init() <- puts it to NULL
>>>   ....
>>>   {tcp,unix}_start_outgoing_migration ->
>>>      socket_outgoing_migration
>>>         migration_channel_connect()
>>> 	   sets to_dst_file
>>>
>>> if tls is enabled, we do another round through
>>> migrate_channel_tls_connect(), but we only set it up if there is no
>>> error.  So we don't need the assignation.  I am removing it to remove
>>> in the follwing patches the knowledge about MigrationState in that two
>>> files.
>>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>  migration/socket.c | 1 -
>>>  migration/tls.c    | 1 -
>>>  2 files changed, 2 deletions(-)
>>>
>>> diff --git a/migration/socket.c b/migration/socket.c
>>> index 13966f1..dc88812 100644
>>> --- a/migration/socket.c
>>> +++ b/migration/socket.c
>>> @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task,
>>>  
>>>      if (qio_task_propagate_error(task, &err)) {
>>>          trace_migration_socket_outgoing_error(error_get_pretty(err));
>>> -        data->s->to_dst_file = NULL;
>>>          migrate_fd_error(data->s, err);
>>>          error_free(err);
>>>      } else {
>>> diff --git a/migration/tls.c b/migration/tls.c
>>> index 45bec44..a33ecb7 100644
>>> --- a/migration/tls.c
>>> +++ b/migration/tls.c
>>> @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task,
>>>  
>>>      if (qio_task_propagate_error(task, &err)) {
>>>          trace_migration_tls_outgoing_handshake_error(error_get_pretty(err));
>>> -        s->to_dst_file = NULL;
>>>          migrate_fd_error(s, err);
>>>          error_free(err);
>>>      } else {
>>>
>>
>> In migrate_fd_error(), we have "assert(s->to_dst_file == NULL);", so you
>> break the function with this change.
> 
> I missundertood something, or everyway to arrive here, to_dst_file is
> always NULL.  See the comment of the file, the only distintion if we go
> through tls is that we do "yet" another round through the init
> functions, but it is still NULL.  At least I can't see how this can be
> NULL at this point.
> 
> Forget about tls for now, centrate in the normal socket.c case:
> 
> we are inside socket_outgoing_migration()
>     nothing here touch any componetes of data
> 
> so go to the caller:
> 
> socket_start_outgoing_migration().
> 
> It init all data values to NULL/0 (g_new0).
> 
> we call qio_channel_set_name() -> no "data" here.
> qio_channel_socket_connect_async()
> 
> It touches "neworking" things here, but nothing related to data, the
> first function that we call when we receive a connection is
> socket_outgoing_migration(), so we are good.
> 
> 
> Back to the tls case:
> 
> migration_tls_outgoing_handshake () -> nothing touch "s" until we call
> migrate_fd_error().
> 
> what calls migration_tls_outgoing_handshake?
> migration_tls_outgoing_connect().
> 
> But that only calls it using qio_channel_tls_handshake().  Notice that
> they pass us here MigrationState, so, who calls this function?
> 
> migration_channel_connect(), this is the function that calls tls, and
> tls ends calling this function without the tls stuff.  So, at the point
> when we setup s->to_dst_file = f here, it is the 1st time that we have
> set that field, too late to affect the migrate_fd_error() that we were
> talking about, no?

Yes, you're right... I should have read your message more carefully...

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 2/2] migration: to_dst_file at that point is NULL

[Daniel P. Berrange]

On Tue, Apr 25, 2017 at 12:17:58PM +0200, Juan Quintela wrote:
> We have just arrived as:
> 
> migration.c: qemu_migrate()
>   ....
>   s = migrate_init() <- puts it to NULL
>   ....
>   {tcp,unix}_start_outgoing_migration ->
>      socket_outgoing_migration
>         migration_channel_connect()
> 	   sets to_dst_file
> 
> if tls is enabled, we do another round through
> migrate_channel_tls_connect(), but we only set it up if there is no
> error.  So we don't need the assignation.  I am removing it to remove
> in the follwing patches the knowledge about MigrationState in that two
> files.

Yes, the logic is quite subtle, but your analysis looks correct.

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

> ---
>  migration/socket.c | 1 -
>  migration/tls.c    | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/migration/socket.c b/migration/socket.c
> index 13966f1..dc88812 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task,
>  
>      if (qio_task_propagate_error(task, &err)) {
>          trace_migration_socket_outgoing_error(error_get_pretty(err));
> -        data->s->to_dst_file = NULL;
>          migrate_fd_error(data->s, err);
>          error_free(err);
>      } else {
> diff --git a/migration/tls.c b/migration/tls.c
> index 45bec44..a33ecb7 100644
> --- a/migration/tls.c
> +++ b/migration/tls.c
> @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task,
>  
>      if (qio_task_propagate_error(task, &err)) {
>          trace_migration_tls_outgoing_handshake_error(error_get_pretty(err));
> -        s->to_dst_file = NULL;
>          migrate_fd_error(s, err);
>          error_free(err);
>      } else {

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 2/2] migration: to_dst_file at that point is NULL

[Peter Xu]

On Tue, Apr 25, 2017 at 12:17:58PM +0200, Juan Quintela wrote:
> We have just arrived as:
> 
> migration.c: qemu_migrate()
>   ....
>   s = migrate_init() <- puts it to NULL
>   ....
>   {tcp,unix}_start_outgoing_migration ->
>      socket_outgoing_migration
>         migration_channel_connect()
> 	   sets to_dst_file
> 
> if tls is enabled, we do another round through
> migrate_channel_tls_connect(), but we only set it up if there is no
> error.  So we don't need the assignation.  I am removing it to remove
> in the follwing patches the knowledge about MigrationState in that two
> files.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

> ---
>  migration/socket.c | 1 -
>  migration/tls.c    | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/migration/socket.c b/migration/socket.c
> index 13966f1..dc88812 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task,
>  
>      if (qio_task_propagate_error(task, &err)) {
>          trace_migration_socket_outgoing_error(error_get_pretty(err));
> -        data->s->to_dst_file = NULL;
>          migrate_fd_error(data->s, err);
>          error_free(err);
>      } else {
> diff --git a/migration/tls.c b/migration/tls.c
> index 45bec44..a33ecb7 100644
> --- a/migration/tls.c
> +++ b/migration/tls.c
> @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task,
>  
>      if (qio_task_propagate_error(task, &err)) {
>          trace_migration_tls_outgoing_handshake_error(error_get_pretty(err));
> -        s->to_dst_file = NULL;
>          migrate_fd_error(s, err);
>          error_free(err);
>      } else {
> -- 
> 2.9.3
> 

-- 
Peter Xu