[Qemu-devel] [PATCH v3 0/3] -incoming defer

[Qemu-devel] [PATCH v3 0/3] -incoming defer

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This patchset provides a way of setting options on an incoming
migration before the fd/process/socket has been created.

   start qemu with   -incoming defer
   <use the monitor to set whatever you need>
   migrate_incoming theuri

v3:
  s/pause/defer/  (and associated fixups)
  Add ' around the '-incoming defer' in an error

v2:
  Create migrate_incoming/migrate-incoming rather than squashing the -u
  into the existing migrate command.

Dave

Dr. David Alan Gilbert (3):
  Add -incoming defer
  Add migrate_incoming
  Document -incoming options

 hmp-commands.hx       | 16 ++++++++++++++++
 hmp.c                 | 14 ++++++++++++++
 hmp.h                 |  1 +
 migration/migration.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 qapi-schema.json      | 15 +++++++++++++++
 qemu-options.hx       | 29 ++++++++++++++++++++++++++---
 qmp-commands.hx       | 31 ++++++++++++++++++++++++++++++-
 7 files changed, 144 insertions(+), 10 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v3 1/3] Add -incoming defer

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

-incoming defer causes qemu to wait for an incoming migration
to be specified later.  The monitor can be used to set migration
capabilities that may affect the incoming connection process.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b3adbc6..f3d49d5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -49,6 +49,8 @@ enum {
 static NotifierList migration_state_notifiers =
     NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
 
+static bool deferred_incoming;
+
 /* When we add fault tolerance, we could have several
    migrations at once.  For now we don't need to add
    dynamic creation of migration */
@@ -65,25 +67,40 @@ MigrationState *migrate_get_current(void)
     return &current_migration;
 }
 
+/*
+ * Called on -incoming with a defer: uri.
+ * The migration can be started later after any parameters have been
+ * changed.
+ */
+static void deferred_incoming_migration(Error **errp)
+{
+    if (deferred_incoming) {
+        error_setg(errp, "Incoming migration already deferred");
+    }
+    deferred_incoming = true;
+}
+
 void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p;
 
-    if (strstart(uri, "tcp:", &p))
+    if (!strcmp(uri, "defer")) {
+        deferred_incoming_migration(errp);
+    } else if (strstart(uri, "tcp:", &p)) {
         tcp_start_incoming_migration(p, errp);
 #ifdef CONFIG_RDMA
-    else if (strstart(uri, "rdma:", &p))
+    } else if (strstart(uri, "rdma:", &p)) {
         rdma_start_incoming_migration(p, errp);
 #endif
 #if !defined(WIN32)
-    else if (strstart(uri, "exec:", &p))
+    } else if (strstart(uri, "exec:", &p)) {
         exec_start_incoming_migration(p, errp);
-    else if (strstart(uri, "unix:", &p))
+    } else if (strstart(uri, "unix:", &p)) {
         unix_start_incoming_migration(p, errp);
-    else if (strstart(uri, "fd:", &p))
+    } else if (strstart(uri, "fd:", &p)) {
         fd_start_incoming_migration(p, errp);
 #endif
-    else {
+    } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
 }
-- 
2.1.0

[Qemu-devel] [PATCH v3 2/3] Add migrate_incoming

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add migrate_incoming/migrate-incoming to start an incoming
migration.

Once a qemu has been started with
    -incoming defer

the migration can be started by issuing:
    migrate_incoming uri

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hmp-commands.hx       | 16 ++++++++++++++++
 hmp.c                 | 14 ++++++++++++++
 hmp.h                 |  1 +
 migration/migration.c | 19 +++++++++++++++++++
 qapi-schema.json      | 15 +++++++++++++++
 qmp-commands.hx       | 31 ++++++++++++++++++++++++++++++-
 6 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index e37bc8b..5dcc556 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -922,6 +922,22 @@ Cancel the current VM migration.
 ETEXI
 
     {
+        .name       = "migrate_incoming",
+        .args_type  = "uri:s",
+        .params     = "uri",
+        .help       = "Continue an incoming migration from an -incoming defer",
+        .mhandler.cmd = hmp_migrate_incoming,
+    },
+
+STEXI
+@item migrate_incoming @var{uri}
+@findex migrate_incoming
+Continue an incoming migration using the @var{uri} (that has the same syntax
+as the -incoming option).
+
+ETEXI
+
+    {
         .name       = "migrate_set_cache_size",
         .args_type  = "value:o",
         .params     = "value",
diff --git a/hmp.c b/hmp.c
index b47f331..f051896 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1083,6 +1083,20 @@ void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
     qmp_migrate_cancel(NULL);
 }
 
+void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    const char *uri = qdict_get_str(qdict, "uri");
+
+    qmp_migrate_incoming(uri, &err);
+
+    if (err) {
+        monitor_printf(mon, "%s\n", error_get_pretty(err));
+        error_free(err);
+        return;
+    }
+}
+
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
 {
     double value = qdict_get_double(qdict, "value");
diff --git a/hmp.h b/hmp.h
index 4bb5dca..95efe63 100644
--- a/hmp.h
+++ b/hmp.h
@@ -60,6 +60,7 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
 void hmp_drive_backup(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
+void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
diff --git a/migration/migration.c b/migration/migration.c
index f3d49d5..2c805f1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -432,6 +432,25 @@ void migrate_del_blocker(Error *reason)
     migration_blockers = g_slist_remove(migration_blockers, reason);
 }
 
+void qmp_migrate_incoming(const char *uri, Error **errp)
+{
+    Error *local_err = NULL;
+
+    if (!deferred_incoming) {
+        error_setg(errp, "'-incoming defer' is required for migrate_incoming");
+        return;
+    }
+
+    qemu_start_incoming_migration(uri, &local_err);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    deferred_incoming = false;
+}
+
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  Error **errp)
diff --git a/qapi-schema.json b/qapi-schema.json
index e16f8eb..7a80081 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1738,6 +1738,21 @@
 { 'command': 'migrate',
   'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
 
+##
+# @migrate-incoming
+#
+# Start an incoming migration, the qemu must have been started
+# with -incoming defer
+#
+# @uri: The Uniform Resource Identifier identifying the source or
+#       address to listen on
+#
+# Returns: nothing on success
+#
+# Since: 2.3
+##
+{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
+
 # @xen-save-devices-state:
 #
 # Save the state of all devices to file. The RAM and the block devices
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a85d847..60181c7 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -661,7 +661,36 @@ Example:
 <- { "return": {} }
 
 EQMP
-{
+
+    {
+        .name       = "migrate-incoming",
+        .args_type  = "uri:s",
+        .mhandler.cmd_new = qmp_marshal_input_migrate_incoming,
+    },
+
+SQMP
+migrate-incoming
+----------------
+
+Continue an incoming migration
+
+Arguments:
+
+- "uri": Source/listening URI (json-string)
+
+Example:
+
+-> { "execute": "migrate-incoming", "arguments": { "uri": "tcp::4446" } }
+<- { "return": {} }
+
+Notes:
+
+(1) QEMU must be started with -incoming defer to allow migrate-incoming to
+    be used
+(2) The uri format is the same as to -incoming
+
+EQMP
+    {
         .name       = "migrate-set-cache-size",
         .args_type  = "value:o",
         .mhandler.cmd_new = qmp_marshal_input_migrate_set_cache_size,
-- 
2.1.0

[Qemu-devel] [PATCH v3 3/3] Document -incoming options

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Document the various URI formats for -incoming, the previous
manpage and help text was wrong (out of date?)

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 qemu-options.hx | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 85ca3ad..6d6d2a8 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3169,12 +3169,35 @@ Set TB size.
 ETEXI
 
 DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
-    "-incoming p     prepare for incoming migration, listen on port p\n",
+    "-incoming uri   prepare for incoming migration, specifying source:\n" \
+    "                exec:command    Execute 'command' use the stdout as\n" \
+    "                                the migration stream\n" \
+    "                fd:num          listen on the given fd\n" \
+    "                defer           wait for the URI to be specified by\n" \
+    "                                the monitor (migrate_incoming)\n" \
+    "                rdma:addr:port  Listen on RDMA port on given address\n" \
+    "                tcp:addr:port   listen on TCP port (optional address)\n" \
+    "                unix:path       listen on the UNIX socket 'path'\n", \
     QEMU_ARCH_ALL)
 STEXI
-@item -incoming @var{port}
+@item -incoming @var{uri}
 @findex -incoming
-Prepare for incoming migration, listen on @var{port}.
+Prepare for incoming migration, specifying the source of the migration stream
+@table @option
+@item exec:@var{command}
+Execute 'command' and use the stdout as the migration stream.
+@item fd:@var{num}
+listen on the given fd
+@item defer
+wait for the URI to be specified by the monitor (migrate_incoming)
+@item rdma:@var{addr}:@var{port}
+Listen on RDMA port on given address
+@item tcp:@var{addr}:@var{port}[,ipv4][,ipv6][,to=to]
+Listen on TCP port @var{port} (optional @var{addr} to specify address to listen on).
+The options ,ipv4, ipv6 and ,to are used in the same manner as chardev TCP options.
+@item unix:@var{path}
+listen on the UNIX socket @var{path}
+@end table
 ETEXI
 
 DEF("nodefaults", 0, QEMU_OPTION_nodefaults, \
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 1/3] Add -incoming defer

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> -incoming defer causes qemu to wait for an incoming migration
> to be specified later.  The monitor can be used to set migration
> capabilities that may affect the incoming connection process.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index b3adbc6..f3d49d5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -49,6 +49,8 @@ enum {
>  static NotifierList migration_state_notifiers =
>      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>  
> +static bool deferred_incoming;
> +
>  /* When we add fault tolerance, we could have several
>     migrations at once.  For now we don't need to add
>     dynamic creation of migration */
> @@ -65,25 +67,40 @@ MigrationState *migrate_get_current(void)
>      return &current_migration;
>  }
>  
> +/*
> + * Called on -incoming with a defer: uri.

The colon in "defer:" is inaccurate, because...

> + * The migration can be started later after any parameters have been
> + * changed.
> + */
> +static void deferred_incoming_migration(Error **errp)
> +{
> +    if (deferred_incoming) {
> +        error_setg(errp, "Incoming migration already deferred");
> +    }
> +    deferred_incoming = true;
> +}
> +
>  void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p;
>  
> -    if (strstart(uri, "tcp:", &p))
> +    if (!strcmp(uri, "defer")) {

... you recognize exactly "defer" here.  <pedantic>Which makes it not an
URI</pedantic>.

> +        deferred_incoming_migration(errp);
> +    } else if (strstart(uri, "tcp:", &p)) {
>          tcp_start_incoming_migration(p, errp);
>  #ifdef CONFIG_RDMA
> -    else if (strstart(uri, "rdma:", &p))
> +    } else if (strstart(uri, "rdma:", &p)) {
>          rdma_start_incoming_migration(p, errp);
>  #endif
>  #if !defined(WIN32)
> -    else if (strstart(uri, "exec:", &p))
> +    } else if (strstart(uri, "exec:", &p)) {
>          exec_start_incoming_migration(p, errp);
> -    else if (strstart(uri, "unix:", &p))
> +    } else if (strstart(uri, "unix:", &p)) {
>          unix_start_incoming_migration(p, errp);
> -    else if (strstart(uri, "fd:", &p))
> +    } else if (strstart(uri, "fd:", &p)) {
>          fd_start_incoming_migration(p, errp);
>  #endif
> -    else {
> +    } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
>  }

How did you test the new error?

I tried, but ran into this preexisting bug:

    $ qemu-system-x86_64 -nodefaults -S -display none -incoming defer -incoming defer
    ERROR: invalid runstate transition: 'inmigrate' -> 'inmigrate'
    Aborted (core dumped)

In my opinion, multiple -incoming should behave like command line
options usually do: last one wins silently.

Re: [Qemu-devel] [PATCH v3 2/3] Add migrate_incoming

[Markus Armbruster]

I'd like Eric's opinion on on encoding configuration tuples as URIs
rather than JSON in QMP.

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Add migrate_incoming/migrate-incoming to start an incoming
> migration.
>
> Once a qemu has been started with
>     -incoming defer
>
> the migration can be started by issuing:
>     migrate_incoming uri
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  hmp-commands.hx       | 16 ++++++++++++++++
>  hmp.c                 | 14 ++++++++++++++
>  hmp.h                 |  1 +
>  migration/migration.c | 19 +++++++++++++++++++
>  qapi-schema.json      | 15 +++++++++++++++
>  qmp-commands.hx       | 31 ++++++++++++++++++++++++++++++-
>  6 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e37bc8b..5dcc556 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -922,6 +922,22 @@ Cancel the current VM migration.
>  ETEXI
>  
>      {
> +        .name       = "migrate_incoming",
> +        .args_type  = "uri:s",
> +        .params     = "uri",
> +        .help       = "Continue an incoming migration from an -incoming defer",
> +        .mhandler.cmd = hmp_migrate_incoming,
> +    },
> +
> +STEXI
> +@item migrate_incoming @var{uri}
> +@findex migrate_incoming
> +Continue an incoming migration using the @var{uri} (that has the same syntax
> +as the -incoming option).
> +
> +ETEXI
> +
> +    {
>          .name       = "migrate_set_cache_size",
>          .args_type  = "value:o",
>          .params     = "value",
> diff --git a/hmp.c b/hmp.c
> index b47f331..f051896 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1083,6 +1083,20 @@ void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
>      qmp_migrate_cancel(NULL);
>  }
>  
> +void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    const char *uri = qdict_get_str(qdict, "uri");
> +
> +    qmp_migrate_incoming(uri, &err);
> +
> +    if (err) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(err));
> +        error_free(err);
> +        return;
> +    }

Please replace the conditional by

       hmp_handle_error(mon, err);

I'll clean up the other places that should use it.

> +}
> +
>  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
>  {
>      double value = qdict_get_double(qdict, "value");
> diff --git a/hmp.h b/hmp.h
> index 4bb5dca..95efe63 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -60,6 +60,7 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
>  void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
>  void hmp_drive_backup(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
> +void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
> diff --git a/migration/migration.c b/migration/migration.c
> index f3d49d5..2c805f1 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -432,6 +432,25 @@ void migrate_del_blocker(Error *reason)
>      migration_blockers = g_slist_remove(migration_blockers, reason);
>  }
>  
> +void qmp_migrate_incoming(const char *uri, Error **errp)
> +{
> +    Error *local_err = NULL;
> +
> +    if (!deferred_incoming) {
> +        error_setg(errp, "'-incoming defer' is required for migrate_incoming");
> +        return;
> +    }
> +
> +    qemu_start_incoming_migration(uri, &local_err);
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    deferred_incoming = false;
> +}
> +

The error message refers to the command as "migrate_incoming", which is
wrong for QMP.

Apart from that, the error message is fine when we fail because the user
didn't specify -incoming defer.  It's unhelpful when we fail because
migrate-incoming has already been run.

You could try something like

    void qmp_migrate_incoming(const char *uri, Error **errp)
    {
        Error *local_err = NULL;

        if (!deferred_incoming) {
            error_setg(errp, "'-incoming defer' is required");
            return;
        }
        if (!runstate_check(RUN_STATE_INMIGRATE)) {
            error_setg(errp, "No migration incoming"
            return;
        }

        qemu_start_incoming_migration(uri, &local_err);

        if (local_err) {
            error_propagate(errp, local_err);
            return;
        }
    }

You're welcome to improve my rather laconic error messages.

>  void qmp_migrate(const char *uri, bool has_blk, bool blk,
>                   bool has_inc, bool inc, bool has_detach, bool detach,
>                   Error **errp)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index e16f8eb..7a80081 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1738,6 +1738,21 @@
>  { 'command': 'migrate',
>    'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
>  
> +##
> +# @migrate-incoming
> +#
> +# Start an incoming migration, the qemu must have been started
> +# with -incoming defer
> +#
> +# @uri: The Uniform Resource Identifier identifying the source or
> +#       address to listen on
> +#
> +# Returns: nothing on success
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
> +
>  # @xen-save-devices-state:
>  #
>  # Save the state of all devices to file. The RAM and the block devices

Eric, what's your take on this?

The general rule in QMP is "no ad hoc encoding of tuples in strings, use
JSON objects instead".

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index a85d847..60181c7 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -661,7 +661,36 @@ Example:
>  <- { "return": {} }
>  
>  EQMP
> -{
> +
> +    {
> +        .name       = "migrate-incoming",
> +        .args_type  = "uri:s",
> +        .mhandler.cmd_new = qmp_marshal_input_migrate_incoming,
> +    },
> +
> +SQMP
> +migrate-incoming
> +----------------
> +
> +Continue an incoming migration
> +
> +Arguments:
> +
> +- "uri": Source/listening URI (json-string)
> +
> +Example:
> +
> +-> { "execute": "migrate-incoming", "arguments": { "uri": "tcp::4446" } }
> +<- { "return": {} }
> +
> +Notes:
> +
> +(1) QEMU must be started with -incoming defer to allow migrate-incoming to
> +    be used
> +(2) The uri format is the same as to -incoming
> +
> +EQMP
> +    {
>          .name       = "migrate-set-cache-size",
>          .args_type  = "value:o",
>          .mhandler.cmd_new = qmp_marshal_input_migrate_set_cache_size,

Re: [Qemu-devel] [PATCH v3 1/3] Add -incoming defer

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > -incoming defer causes qemu to wait for an incoming migration
> > to be specified later.  The monitor can be used to set migration
> > capabilities that may affect the incoming connection process.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > ---
> >  migration/migration.c | 29 +++++++++++++++++++++++------
> >  1 file changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index b3adbc6..f3d49d5 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -49,6 +49,8 @@ enum {
> >  static NotifierList migration_state_notifiers =
> >      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> >  
> > +static bool deferred_incoming;
> > +
> >  /* When we add fault tolerance, we could have several
> >     migrations at once.  For now we don't need to add
> >     dynamic creation of migration */
> > @@ -65,25 +67,40 @@ MigrationState *migrate_get_current(void)
> >      return &current_migration;
> >  }
> >  
> > +/*
> > + * Called on -incoming with a defer: uri.
> 
> The colon in "defer:" is inaccurate, because...

True, but probably not worth the respin.

> > + * The migration can be started later after any parameters have been
> > + * changed.
> > + */
> > +static void deferred_incoming_migration(Error **errp)
> > +{
> > +    if (deferred_incoming) {
> > +        error_setg(errp, "Incoming migration already deferred");
> > +    }
> > +    deferred_incoming = true;
> > +}
> > +
> >  void qemu_start_incoming_migration(const char *uri, Error **errp)
> >  {
> >      const char *p;
> >  
> > -    if (strstart(uri, "tcp:", &p))
> > +    if (!strcmp(uri, "defer")) {
> 
> ... you recognize exactly "defer" here.  <pedantic>Which makes it not an
> URI</pedantic>.
> 
> > +        deferred_incoming_migration(errp);
> > +    } else if (strstart(uri, "tcp:", &p)) {
> >          tcp_start_incoming_migration(p, errp);
> >  #ifdef CONFIG_RDMA
> > -    else if (strstart(uri, "rdma:", &p))
> > +    } else if (strstart(uri, "rdma:", &p)) {
> >          rdma_start_incoming_migration(p, errp);
> >  #endif
> >  #if !defined(WIN32)
> > -    else if (strstart(uri, "exec:", &p))
> > +    } else if (strstart(uri, "exec:", &p)) {
> >          exec_start_incoming_migration(p, errp);
> > -    else if (strstart(uri, "unix:", &p))
> > +    } else if (strstart(uri, "unix:", &p)) {
> >          unix_start_incoming_migration(p, errp);
> > -    else if (strstart(uri, "fd:", &p))
> > +    } else if (strstart(uri, "fd:", &p)) {
> >          fd_start_incoming_migration(p, errp);
> >  #endif
> > -    else {
> > +    } else {
> >          error_setg(errp, "unknown migration protocol: %s", uri);
> >      }
> >  }
> 
> How did you test the new error?
> 
> I tried, but ran into this preexisting bug:
> 
>     $ qemu-system-x86_64 -nodefaults -S -display none -incoming defer -incoming defer
>     ERROR: invalid runstate transition: 'inmigrate' -> 'inmigrate'
>     Aborted (core dumped)

Fun; yes that's an existing bug that triggers with -incoming tcp::4444 -incoming tcp::4445 
I'll think about that separately.

The way I tested the error was like this:

$ bin/qemu-system-x86_64 -nographic -incoming defer
QEMU 2.2.50 monitor - type 'help' for more information                                                                                                                                                                                       
(qemu) migrate_incoming defer
Incoming migration already deferred                                                                                                                                                                                                          
(qemu)

> In my opinion, multiple -incoming should behave like command line
> options usually do: last one wins silently.

Either that or error; to me this feels like it should probably
error; I'd make the distinction between options that set a value
(which works as a last one wins), or options that invoke an action,
for options that invoke an action it feels wrong to me to allow
incompatible options.

Dave



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 2/3] Add migrate_incoming

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> I'd like Eric's opinion on on encoding configuration tuples as URIs
> rather than JSON in QMP.

Old discussion; he's already R-b the previous version of this
patch where the only difference was pause/defer.

Dave

> 
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Add migrate_incoming/migrate-incoming to start an incoming
> > migration.
> >
> > Once a qemu has been started with
> >     -incoming defer
> >
> > the migration can be started by issuing:
> >     migrate_incoming uri
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  hmp-commands.hx       | 16 ++++++++++++++++
> >  hmp.c                 | 14 ++++++++++++++
> >  hmp.h                 |  1 +
> >  migration/migration.c | 19 +++++++++++++++++++
> >  qapi-schema.json      | 15 +++++++++++++++
> >  qmp-commands.hx       | 31 ++++++++++++++++++++++++++++++-
> >  6 files changed, 95 insertions(+), 1 deletion(-)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index e37bc8b..5dcc556 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -922,6 +922,22 @@ Cancel the current VM migration.
> >  ETEXI
> >  
> >      {
> > +        .name       = "migrate_incoming",
> > +        .args_type  = "uri:s",
> > +        .params     = "uri",
> > +        .help       = "Continue an incoming migration from an -incoming defer",
> > +        .mhandler.cmd = hmp_migrate_incoming,
> > +    },
> > +
> > +STEXI
> > +@item migrate_incoming @var{uri}
> > +@findex migrate_incoming
> > +Continue an incoming migration using the @var{uri} (that has the same syntax
> > +as the -incoming option).
> > +
> > +ETEXI
> > +
> > +    {
> >          .name       = "migrate_set_cache_size",
> >          .args_type  = "value:o",
> >          .params     = "value",
> > diff --git a/hmp.c b/hmp.c
> > index b47f331..f051896 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1083,6 +1083,20 @@ void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
> >      qmp_migrate_cancel(NULL);
> >  }
> >  
> > +void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    const char *uri = qdict_get_str(qdict, "uri");
> > +
> > +    qmp_migrate_incoming(uri, &err);
> > +
> > +    if (err) {
> > +        monitor_printf(mon, "%s\n", error_get_pretty(err));
> > +        error_free(err);
> > +        return;
> > +    }
> 
> Please replace the conditional by
> 
>        hmp_handle_error(mon, err);
> 
> I'll clean up the other places that should use it.
> 
> > +}
> > +
> >  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
> >  {
> >      double value = qdict_get_double(qdict, "value");
> > diff --git a/hmp.h b/hmp.h
> > index 4bb5dca..95efe63 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -60,6 +60,7 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
> >  void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
> >  void hmp_drive_backup(Monitor *mon, const QDict *qdict);
> >  void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
> > +void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
> >  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
> >  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
> >  void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index f3d49d5..2c805f1 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -432,6 +432,25 @@ void migrate_del_blocker(Error *reason)
> >      migration_blockers = g_slist_remove(migration_blockers, reason);
> >  }
> >  
> > +void qmp_migrate_incoming(const char *uri, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +
> > +    if (!deferred_incoming) {
> > +        error_setg(errp, "'-incoming defer' is required for migrate_incoming");
> > +        return;
> > +    }
> > +
> > +    qemu_start_incoming_migration(uri, &local_err);
> > +
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    deferred_incoming = false;
> > +}
> > +
> 
> The error message refers to the command as "migrate_incoming", which is
> wrong for QMP.
> 
> Apart from that, the error message is fine when we fail because the user
> didn't specify -incoming defer.  It's unhelpful when we fail because
> migrate-incoming has already been run.
> 
> You could try something like
> 
>     void qmp_migrate_incoming(const char *uri, Error **errp)
>     {
>         Error *local_err = NULL;
> 
>         if (!deferred_incoming) {
>             error_setg(errp, "'-incoming defer' is required");
>             return;
>         }
>         if (!runstate_check(RUN_STATE_INMIGRATE)) {
>             error_setg(errp, "No migration incoming"
>             return;
>         }
> 
>         qemu_start_incoming_migration(uri, &local_err);
> 
>         if (local_err) {
>             error_propagate(errp, local_err);
>             return;
>         }
>     }
> 
> You're welcome to improve my rather laconic error messages.
> 
> >  void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >                   bool has_inc, bool inc, bool has_detach, bool detach,
> >                   Error **errp)
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index e16f8eb..7a80081 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1738,6 +1738,21 @@
> >  { 'command': 'migrate',
> >    'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
> >  
> > +##
> > +# @migrate-incoming
> > +#
> > +# Start an incoming migration, the qemu must have been started
> > +# with -incoming defer
> > +#
> > +# @uri: The Uniform Resource Identifier identifying the source or
> > +#       address to listen on
> > +#
> > +# Returns: nothing on success
> > +#
> > +# Since: 2.3
> > +##
> > +{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
> > +
> >  # @xen-save-devices-state:
> >  #
> >  # Save the state of all devices to file. The RAM and the block devices
> 
> Eric, what's your take on this?
> 
> The general rule in QMP is "no ad hoc encoding of tuples in strings, use
> JSON objects instead".
> 
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index a85d847..60181c7 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -661,7 +661,36 @@ Example:
> >  <- { "return": {} }
> >  
> >  EQMP
> > -{
> > +
> > +    {
> > +        .name       = "migrate-incoming",
> > +        .args_type  = "uri:s",
> > +        .mhandler.cmd_new = qmp_marshal_input_migrate_incoming,
> > +    },
> > +
> > +SQMP
> > +migrate-incoming
> > +----------------
> > +
> > +Continue an incoming migration
> > +
> > +Arguments:
> > +
> > +- "uri": Source/listening URI (json-string)
> > +
> > +Example:
> > +
> > +-> { "execute": "migrate-incoming", "arguments": { "uri": "tcp::4446" } }
> > +<- { "return": {} }
> > +
> > +Notes:
> > +
> > +(1) QEMU must be started with -incoming defer to allow migrate-incoming to
> > +    be used
> > +(2) The uri format is the same as to -incoming
> > +
> > +EQMP
> > +    {
> >          .name       = "migrate-set-cache-size",
> >          .args_type  = "value:o",
> >          .mhandler.cmd_new = qmp_marshal_input_migrate_set_cache_size,
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 3/3] Document -incoming options

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Document the various URI formats for -incoming, the previous
> manpage and help text was wrong (out of date?)

Thanks a lot for updating the docs.

>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> ---
>  qemu-options.hx | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 85ca3ad..6d6d2a8 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3169,12 +3169,35 @@ Set TB size.
>  ETEXI
>  
>  DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
> -    "-incoming p     prepare for incoming migration, listen on port p\n",
> +    "-incoming uri   prepare for incoming migration, specifying source:\n" \
> +    "                exec:command    Execute 'command' use the stdout as\n" \
> +    "                                the migration stream\n" \
> +    "                fd:num          listen on the given fd\n" \
> +    "                defer           wait for the URI to be specified by\n" \
> +    "                                the monitor (migrate_incoming)\n" \
> +    "                rdma:addr:port  Listen on RDMA port on given address\n" \
> +    "                tcp:addr:port   listen on TCP port (optional address)\n" \
> +    "                unix:path       listen on the UNIX socket 'path'\n", \
>      QEMU_ARCH_ALL)
>  STEXI
> -@item -incoming @var{port}
> +@item -incoming @var{uri}
>  @findex -incoming
> -Prepare for incoming migration, listen on @var{port}.
> +Prepare for incoming migration, specifying the source of the migration stream
> +@table @option
> +@item exec:@var{command}
> +Execute 'command' and use the stdout as the migration stream.

Suggest:

    Execute 'command' and use its standard output as migration stream
    source.

> +@item fd:@var{num}
> +listen on the given fd

Well, we're not listening in usual the sense of listen(2).  Suggest

    Use file descriptor @var{num}.

> +@item defer
> +wait for the URI to be specified by the monitor (migrate_incoming)

"Wait", since the other items start with a capital letter.

> +@item rdma:@var{addr}:@var{port}
> +Listen on RDMA port on given address

Let's call the thing between the colons "host" instead of "äddr".  The
address consists of host and port.

rdma_start_incoming_migration() passes everything after rdma: to
qemu_rdma_data_init(), which parses it with inet_parse(), then uses only
host and port.  The other members of InetSocketAddress are silently
ignored.  Wonderful.

qemu_rdma_data_init() errors out if you omit host.

inet_parse() accepts one of

    :PORT
    [V6ADDR]:PORT
    V4ADDR:PORT
    HOST:PORT

followed by an options string.  The options string isn't really parsed,
instead substrings matching one of these patterns are recognized:

    ,to=PORT
    ,ipv4
    ,ipv6

Note: you can specify ipv4,ipv6 or combine V6ADDR with ipv4, or V4ADDR
with ipv6 to get both flags set in InetSocketAddress.

> +@item tcp:@var{addr}:@var{port}[,ipv4][,ipv6][,to=to]

[@var{host}] instead of @var{addr}, and [,to=@var{port}] instead of
[,to=to], please.

> +Listen on TCP port @var{port} (optional @var{addr} to specify address to listen on).
> +The options ,ipv4, ipv6 and ,to are used in the same manner as chardev TCP options.

The reference to chardev options makes sense, but the reader needs to
make the connection from "chardev TCP options" to "-chardev socket"
himself.  Maybe "in the same manner as in -chardev socket"?

> +@item unix:@var{path}
> +listen on the UNIX socket @var{path}
> +@end table
>  ETEXI
>  
>  DEF("nodefaults", 0, QEMU_OPTION_nodefaults, \

Don't get fooled by my many comments, this series is good stuff!

Re: [Qemu-devel] [PATCH v3 1/3] Add -incoming defer

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> 
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >
>> > -incoming defer causes qemu to wait for an incoming migration
>> > to be specified later.  The monitor can be used to set migration
>> > capabilities that may affect the incoming connection process.
>> >
>> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> > Reviewed-by: Juan Quintela <quintela@redhat.com>
>> > ---
>> >  migration/migration.c | 29 +++++++++++++++++++++++------
>> >  1 file changed, 23 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index b3adbc6..f3d49d5 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -49,6 +49,8 @@ enum {
>> >  static NotifierList migration_state_notifiers =
>> >      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>> >  
>> > +static bool deferred_incoming;
>> > +
>> >  /* When we add fault tolerance, we could have several
>> >     migrations at once.  For now we don't need to add
>> >     dynamic creation of migration */
>> > @@ -65,25 +67,40 @@ MigrationState *migrate_get_current(void)
>> >      return &current_migration;
>> >  }
>> >  
>> > +/*
>> > + * Called on -incoming with a defer: uri.
>> 
>> The colon in "defer:" is inaccurate, because...
>
> True, but probably not worth the respin.
>
>> > + * The migration can be started later after any parameters have been
>> > + * changed.
>> > + */
>> > +static void deferred_incoming_migration(Error **errp)
>> > +{
>> > +    if (deferred_incoming) {
>> > +        error_setg(errp, "Incoming migration already deferred");
>> > +    }
>> > +    deferred_incoming = true;
>> > +}
>> > +
>> >  void qemu_start_incoming_migration(const char *uri, Error **errp)
>> >  {
>> >      const char *p;
>> >  
>> > -    if (strstart(uri, "tcp:", &p))
>> > +    if (!strcmp(uri, "defer")) {
>> 
>> ... you recognize exactly "defer" here.  <pedantic>Which makes it not an
>> URI</pedantic>.
>> 
>> > +        deferred_incoming_migration(errp);
>> > +    } else if (strstart(uri, "tcp:", &p)) {
>> >          tcp_start_incoming_migration(p, errp);
>> >  #ifdef CONFIG_RDMA
>> > -    else if (strstart(uri, "rdma:", &p))
>> > +    } else if (strstart(uri, "rdma:", &p)) {
>> >          rdma_start_incoming_migration(p, errp);
>> >  #endif
>> >  #if !defined(WIN32)
>> > -    else if (strstart(uri, "exec:", &p))
>> > +    } else if (strstart(uri, "exec:", &p)) {
>> >          exec_start_incoming_migration(p, errp);
>> > -    else if (strstart(uri, "unix:", &p))
>> > +    } else if (strstart(uri, "unix:", &p)) {
>> >          unix_start_incoming_migration(p, errp);
>> > -    else if (strstart(uri, "fd:", &p))
>> > +    } else if (strstart(uri, "fd:", &p)) {
>> >          fd_start_incoming_migration(p, errp);
>> >  #endif
>> > -    else {
>> > +    } else {
>> >          error_setg(errp, "unknown migration protocol: %s", uri);
>> >      }
>> >  }
>> 
>> How did you test the new error?
>> 
>> I tried, but ran into this preexisting bug:
>> 
>>     $ qemu-system-x86_64 -nodefaults -S -display none -incoming
>> defer -incoming defer
>>     ERROR: invalid runstate transition: 'inmigrate' -> 'inmigrate'
>>     Aborted (core dumped)
>
> Fun; yes that's an existing bug that triggers with -incoming tcp::4444
> -incoming tcp::4445
> I'll think about that separately.

Makes sense.

> The way I tested the error was like this:
>
> $ bin/qemu-system-x86_64 -nographic -incoming defer
> QEMU 2.2.50 monitor - type 'help' for more information
> (qemu) migrate_incoming defer
> Incoming migration already deferred
> (qemu)
>
>> In my opinion, multiple -incoming should behave like command line
>> options usually do: last one wins silently.
>
> Either that or error; to me this feels like it should probably
> error; I'd make the distinction between options that set a value
> (which works as a last one wins), or options that invoke an action,
> for options that invoke an action it feels wrong to me to allow
> incompatible options.

One man's action is another man's value.

There's ample precedence for "last one wins" when command line options
conflict.  For instance, ls -l conflicts with -C, -m and -x, and POSIX
explicitly specifies "last one wins".  Rather convenient when you almost
always want one option, and use alias to get it without having to type
it all the time, but occasionally want a conflicting option, and can
just give it despite your alias.

Re: [Qemu-devel] [PATCH v3 2/3] Add migrate_incoming

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> I'd like Eric's opinion on on encoding configuration tuples as URIs
>> rather than JSON in QMP.
>
> Old discussion; he's already R-b the previous version of this
> patch where the only difference was pause/defer.

i will send the pull request for this (actually previous version was on
my list already).  Please update Markus comment as a separate serie.

Thanks, Juan.

>
> Dave
>
>> 
>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> 
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >
>> > Add migrate_incoming/migrate-incoming to start an incoming
>> > migration.
>> >
>> > Once a qemu has been started with
>> >     -incoming defer
>> >
>> > the migration can be started by issuing:
>> >     migrate_incoming uri
>> >
>> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> > Reviewed-by: Eric Blake <eblake@redhat.com>
>> > ---
>> >  hmp-commands.hx       | 16 ++++++++++++++++
>> >  hmp.c                 | 14 ++++++++++++++
>> >  hmp.h                 |  1 +
>> >  migration/migration.c | 19 +++++++++++++++++++
>> >  qapi-schema.json      | 15 +++++++++++++++
>> >  qmp-commands.hx       | 31 ++++++++++++++++++++++++++++++-
>> >  6 files changed, 95 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hmp-commands.hx b/hmp-commands.hx
>> > index e37bc8b..5dcc556 100644
>> > --- a/hmp-commands.hx
>> > +++ b/hmp-commands.hx
>> > @@ -922,6 +922,22 @@ Cancel the current VM migration.
>> >  ETEXI
>> >  
>> >      {
>> > +        .name       = "migrate_incoming",
>> > +        .args_type  = "uri:s",
>> > +        .params     = "uri",
>> > +        .help       = "Continue an incoming migration from an -incoming defer",
>> > +        .mhandler.cmd = hmp_migrate_incoming,
>> > +    },
>> > +
>> > +STEXI
>> > +@item migrate_incoming @var{uri}
>> > +@findex migrate_incoming
>> > +Continue an incoming migration using the @var{uri} (that has the same syntax
>> > +as the -incoming option).
>> > +
>> > +ETEXI
>> > +
>> > +    {
>> >          .name       = "migrate_set_cache_size",
>> >          .args_type  = "value:o",
>> >          .params     = "value",
>> > diff --git a/hmp.c b/hmp.c
>> > index b47f331..f051896 100644
>> > --- a/hmp.c
>> > +++ b/hmp.c
>> > @@ -1083,6 +1083,20 @@ void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
>> >      qmp_migrate_cancel(NULL);
>> >  }
>> >  
>> > +void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
>> > +{
>> > +    Error *err = NULL;
>> > +    const char *uri = qdict_get_str(qdict, "uri");
>> > +
>> > +    qmp_migrate_incoming(uri, &err);
>> > +
>> > +    if (err) {
>> > +        monitor_printf(mon, "%s\n", error_get_pretty(err));
>> > +        error_free(err);
>> > +        return;
>> > +    }
>> 
>> Please replace the conditional by
>> 
>>        hmp_handle_error(mon, err);
>> 
>> I'll clean up the other places that should use it.
>> 
>> > +}
>> > +
>> >  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
>> >  {
>> >      double value = qdict_get_double(qdict, "value");
>> > diff --git a/hmp.h b/hmp.h
>> > index 4bb5dca..95efe63 100644
>> > --- a/hmp.h
>> > +++ b/hmp.h
>> > @@ -60,6 +60,7 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
>> >  void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
>> >  void hmp_drive_backup(Monitor *mon, const QDict *qdict);
>> >  void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
>> > +void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
>> >  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
>> >  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
>> >  void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index f3d49d5..2c805f1 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -432,6 +432,25 @@ void migrate_del_blocker(Error *reason)
>> >      migration_blockers = g_slist_remove(migration_blockers, reason);
>> >  }
>> >  
>> > +void qmp_migrate_incoming(const char *uri, Error **errp)
>> > +{
>> > +    Error *local_err = NULL;
>> > +
>> > +    if (!deferred_incoming) {
>> > +        error_setg(errp, "'-incoming defer' is required for migrate_incoming");
>> > +        return;
>> > +    }
>> > +
>> > +    qemu_start_incoming_migration(uri, &local_err);
>> > +
>> > +    if (local_err) {
>> > +        error_propagate(errp, local_err);
>> > +        return;
>> > +    }
>> > +
>> > +    deferred_incoming = false;
>> > +}
>> > +
>> 
>> The error message refers to the command as "migrate_incoming", which is
>> wrong for QMP.
>> 
>> Apart from that, the error message is fine when we fail because the user
>> didn't specify -incoming defer.  It's unhelpful when we fail because
>> migrate-incoming has already been run.
>> 
>> You could try something like
>> 
>>     void qmp_migrate_incoming(const char *uri, Error **errp)
>>     {
>>         Error *local_err = NULL;
>> 
>>         if (!deferred_incoming) {
>>             error_setg(errp, "'-incoming defer' is required");
>>             return;
>>         }
>>         if (!runstate_check(RUN_STATE_INMIGRATE)) {
>>             error_setg(errp, "No migration incoming"
>>             return;
>>         }
>> 
>>         qemu_start_incoming_migration(uri, &local_err);
>> 
>>         if (local_err) {
>>             error_propagate(errp, local_err);
>>             return;
>>         }
>>     }
>> 
>> You're welcome to improve my rather laconic error messages.
>> 
>> >  void qmp_migrate(const char *uri, bool has_blk, bool blk,
>> >                   bool has_inc, bool inc, bool has_detach, bool detach,
>> >                   Error **errp)
>> > diff --git a/qapi-schema.json b/qapi-schema.json
>> > index e16f8eb..7a80081 100644
>> > --- a/qapi-schema.json
>> > +++ b/qapi-schema.json
>> > @@ -1738,6 +1738,21 @@
>> >  { 'command': 'migrate',
>> >    'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
>> >  
>> > +##
>> > +# @migrate-incoming
>> > +#
>> > +# Start an incoming migration, the qemu must have been started
>> > +# with -incoming defer
>> > +#
>> > +# @uri: The Uniform Resource Identifier identifying the source or
>> > +#       address to listen on
>> > +#
>> > +# Returns: nothing on success
>> > +#
>> > +# Since: 2.3
>> > +##
>> > +{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
>> > +
>> >  # @xen-save-devices-state:
>> >  #
>> >  # Save the state of all devices to file. The RAM and the block devices
>> 
>> Eric, what's your take on this?
>> 
>> The general rule in QMP is "no ad hoc encoding of tuples in strings, use
>> JSON objects instead".
>> 
>> > diff --git a/qmp-commands.hx b/qmp-commands.hx
>> > index a85d847..60181c7 100644
>> > --- a/qmp-commands.hx
>> > +++ b/qmp-commands.hx
>> > @@ -661,7 +661,36 @@ Example:
>> >  <- { "return": {} }
>> >  
>> >  EQMP
>> > -{
>> > +
>> > +    {
>> > +        .name       = "migrate-incoming",
>> > +        .args_type  = "uri:s",
>> > +        .mhandler.cmd_new = qmp_marshal_input_migrate_incoming,
>> > +    },
>> > +
>> > +SQMP
>> > +migrate-incoming
>> > +----------------
>> > +
>> > +Continue an incoming migration
>> > +
>> > +Arguments:
>> > +
>> > +- "uri": Source/listening URI (json-string)
>> > +
>> > +Example:
>> > +
>> > +-> { "execute": "migrate-incoming", "arguments": { "uri": "tcp::4446" } }
>> > +<- { "return": {} }
>> > +
>> > +Notes:
>> > +
>> > +(1) QEMU must be started with -incoming defer to allow migrate-incoming to
>> > +    be used
>> > +(2) The uri format is the same as to -incoming
>> > +
>> > +EQMP
>> > +    {
>> >          .name       = "migrate-set-cache-size",
>> >          .args_type  = "value:o",
>> >          .mhandler.cmd_new = qmp_marshal_input_migrate_set_cache_size,
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 2/3] Add migrate_incoming

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]

On 02/20/2015 01:18 AM, Markus Armbruster wrote:
> I'd like Eric's opinion on on encoding configuration tuples as URIs
> rather than JSON in QMP.
> 

>> +{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
>> +
>>  # @xen-save-devices-state:
>>  #
>>  # Save the state of all devices to file. The RAM and the block devices
> 
> Eric, what's your take on this?
> 
> The general rule in QMP is "no ad hoc encoding of tuples in strings, use
> JSON objects instead".

Yes, it would be nice to be type-safe, and have a QAPI union with a
discriminator of different URI types along with appropriate accompanying
data.  But it is also new code (both in qemu to parse it, and in libvirt
to generate it), and we already have existing code that knows how to
generate the string-encoded tuple for the -incoming command line
argument that can be reused as-is with the new QMP command as proposed.
 So even though it is less safe, I'm okay with this particular command
using an overloaded string argument.

Down the road, if we WANT to add type-safety, we can make this command
take an anonymous union, where a 'str' value is the compact old style,
and where a dictionary value is the new discriminated union style.  So
we aren't completely locked into non-type-safe string forever, but we
probably won't add a type-safe variant until (if) we ever add a new
migration format that just looks too ugly as a URI based on the
parameters it requires.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/3] Add migrate_incoming

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/20/2015 01:18 AM, Markus Armbruster wrote:
>> I'd like Eric's opinion on on encoding configuration tuples as URIs
>> rather than JSON in QMP.
>> 
>
>>> +{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
>>> +
>>>  # @xen-save-devices-state:
>>>  #
>>>  # Save the state of all devices to file. The RAM and the block devices
>> 
>> Eric, what's your take on this?
>> 
>> The general rule in QMP is "no ad hoc encoding of tuples in strings, use
>> JSON objects instead".
>
> Yes, it would be nice to be type-safe, and have a QAPI union with a
> discriminator of different URI types along with appropriate accompanying
> data.  But it is also new code (both in qemu to parse it, and in libvirt
> to generate it), and we already have existing code that knows how to
> generate the string-encoded tuple for the -incoming command line
> argument that can be reused as-is with the new QMP command as proposed.
>  So even though it is less safe, I'm okay with this particular command
> using an overloaded string argument.
>
> Down the road, if we WANT to add type-safety, we can make this command
> take an anonymous union, where a 'str' value is the compact old style,
> and where a dictionary value is the new discriminated union style.  So
> we aren't completely locked into non-type-safe string forever, but we
> probably won't add a type-safe variant until (if) we ever add a new
> migration format that just looks too ugly as a URI based on the
> parameters it requires.

I can accept this, but I want the rationale in a comment, so this
exception to the QMP rule serves less well as bad example.

Re: [Qemu-devel] [PATCH v3 0/3] -incoming defer

[Michael S. Tsirkin]

On Thu, Feb 19, 2015 at 11:40:26AM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> This patchset provides a way of setting options on an incoming
> migration before the fd/process/socket has been created.
> 
>    start qemu with   -incoming defer
>    <use the monitor to set whatever you need>
>    migrate_incoming theuri
> 
> v3:
>   s/pause/defer/  (and associated fixups)
>   Add ' around the '-incoming defer' in an error
> 
> v2:
>   Create migrate_incoming/migrate-incoming rather than squashing the -u
>   into the existing migrate command.
> 
> Dave
> 
> Dr. David Alan Gilbert (3):
>   Add -incoming defer
>   Add migrate_incoming
>   Document -incoming options
> 
>  hmp-commands.hx       | 16 ++++++++++++++++
>  hmp.c                 | 14 ++++++++++++++
>  hmp.h                 |  1 +
>  migration/migration.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
>  qapi-schema.json      | 15 +++++++++++++++
>  qemu-options.hx       | 29 ++++++++++++++++++++++++++---
>  qmp-commands.hx       | 31 ++++++++++++++++++++++++++++++-
>  7 files changed, 144 insertions(+), 10 deletions(-)

Update qemu-doc.texi as well?


> -- 
> 2.1.0
> 

Re: [Qemu-devel] [PATCH v3 0/3] -incoming defer

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

On Thu, Feb 19, 2015 at 11:40:26AM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> This patchset provides a way of setting options on an incoming
> migration before the fd/process/socket has been created.
> 
>    start qemu with   -incoming defer
>    <use the monitor to set whatever you need>
>    migrate_incoming theuri

Does this patch series break BDRV_O_INCOMING?

blockdev.c:blockdev_init():
  if (runstate_check(RUN_STATE_INMIGRATE)) {
      bdrv_flags |= BDRV_O_INCOMING;
  }

The block layer needs to know about incoming migrations to avoid
accessing or caching disk images while the source host still has them
open.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 0/3] -incoming defer

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@gmail.com) wrote:
> On Thu, Feb 19, 2015 at 11:40:26AM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > This patchset provides a way of setting options on an incoming
> > migration before the fd/process/socket has been created.
> > 
> >    start qemu with   -incoming defer
> >    <use the monitor to set whatever you need>
> >    migrate_incoming theuri
> 
> Does this patch series break BDRV_O_INCOMING?
> 
> blockdev.c:blockdev_init():
>   if (runstate_check(RUN_STATE_INMIGRATE)) {
>       bdrv_flags |= BDRV_O_INCOMING;
>   }
> 
> The block layer needs to know about incoming migrations to avoid
> accessing or caching disk images while the source host still has them
> open.

No, it doesn't break it; it goes into RUN_STATE_INMIGRATE at startup
in the same way that all existing -incoming's do; the state transitions
should be identical.

Dave

> 
> Stefan


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 0/3] -incoming defer

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Thu, Feb 19, 2015 at 11:40:26AM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > This patchset provides a way of setting options on an incoming
> > migration before the fd/process/socket has been created.
> > 
> >    start qemu with   -incoming defer
> >    <use the monitor to set whatever you need>
> >    migrate_incoming theuri
> > 
> > v3:
> >   s/pause/defer/  (and associated fixups)
> >   Add ' around the '-incoming defer' in an error
> > 
> > v2:
> >   Create migrate_incoming/migrate-incoming rather than squashing the -u
> >   into the existing migrate command.
> > 
> > Dave
> > 
> > Dr. David Alan Gilbert (3):
> >   Add -incoming defer
> >   Add migrate_incoming
> >   Document -incoming options
> > 
> >  hmp-commands.hx       | 16 ++++++++++++++++
> >  hmp.c                 | 14 ++++++++++++++
> >  hmp.h                 |  1 +
> >  migration/migration.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
> >  qapi-schema.json      | 15 +++++++++++++++
> >  qemu-options.hx       | 29 ++++++++++++++++++++++++++---
> >  qmp-commands.hx       | 31 ++++++++++++++++++++++++++++++-
> >  7 files changed, 144 insertions(+), 10 deletions(-)
> 
> Update qemu-doc.texi as well?

Well the qemu-doc.html seems to have gained the migrate_incoming command
and proper definitions of -incoming magically without me having to
look at a texi file - I think that description is parsed from one of
the option files somehow, so hopefully I don't need to wrangle any tex.

Dave

> 
> 
> > -- 
> > 2.1.0
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 0/3] -incoming defer

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

On Mon, Feb 23, 2015 at 10:55:53AM +0000, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@gmail.com) wrote:
> > On Thu, Feb 19, 2015 at 11:40:26AM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > This patchset provides a way of setting options on an incoming
> > > migration before the fd/process/socket has been created.
> > > 
> > >    start qemu with   -incoming defer
> > >    <use the monitor to set whatever you need>
> > >    migrate_incoming theuri
> > 
> > Does this patch series break BDRV_O_INCOMING?
> > 
> > blockdev.c:blockdev_init():
> >   if (runstate_check(RUN_STATE_INMIGRATE)) {
> >       bdrv_flags |= BDRV_O_INCOMING;
> >   }
> > 
> > The block layer needs to know about incoming migrations to avoid
> > accessing or caching disk images while the source host still has them
> > open.
> 
> No, it doesn't break it; it goes into RUN_STATE_INMIGRATE at startup
> in the same way that all existing -incoming's do; the state transitions
> should be identical.

Great, thanks for explaining.

I have verified that vl.c:main() sets RUN_STATE_INMIGRATE in all cases:

  case QEMU_OPTION_incoming:
      incoming = optarg;
      runstate_set(RUN_STATE_INMIGRATE);
      break;

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 2/3] Add migrate_incoming

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> I'd like Eric's opinion on on encoding configuration tuples as URIs
> rather than JSON in QMP.

I've just posted a series which should address all the issues in
here; but note that:

> > +void qmp_migrate_incoming(const char *uri, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +
> > +    if (!deferred_incoming) {
> > +        error_setg(errp, "'-incoming defer' is required for migrate_incoming");
> > +        return;
> > +    }
> > +
> > +    qemu_start_incoming_migration(uri, &local_err);
> > +
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    deferred_incoming = false;
> > +}
> > +
> 
> The error message refers to the command as "migrate_incoming", which is
> wrong for QMP.
> 
> Apart from that, the error message is fine when we fail because the user
> didn't specify -incoming defer.  It's unhelpful when we fail because
> migrate-incoming has already been run.
> 
> You could try something like
> 
>     void qmp_migrate_incoming(const char *uri, Error **errp)
>     {
>         Error *local_err = NULL;
> 
>         if (!deferred_incoming) {
>             error_setg(errp, "'-incoming defer' is required");
>             return;
>         }
>         if (!runstate_check(RUN_STATE_INMIGRATE)) {
>             error_setg(errp, "No migration incoming"
>             return;
>         }
> 
>         qemu_start_incoming_migration(uri, &local_err);
> 
>         if (local_err) {
>             error_propagate(errp, local_err);
>             return;
>         }
>     }
> 
> You're welcome to improve my rather laconic error messages.

doesn't work; because qemu_start_incoming_migration returns
immediately, and so:

migrate_incoming tcp::4444
migrate_incoming tcp::4444

fails trying to bind the port twice.  See the new patch series
for my way of fixing it.


Dave

> 
> >  void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >                   bool has_inc, bool inc, bool has_detach, bool detach,
> >                   Error **errp)
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index e16f8eb..7a80081 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1738,6 +1738,21 @@
> >  { 'command': 'migrate',
> >    'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
> >  
> > +##
> > +# @migrate-incoming
> > +#
> > +# Start an incoming migration, the qemu must have been started
> > +# with -incoming defer
> > +#
> > +# @uri: The Uniform Resource Identifier identifying the source or
> > +#       address to listen on
> > +#
> > +# Returns: nothing on success
> > +#
> > +# Since: 2.3
> > +##
> > +{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
> > +
> >  # @xen-save-devices-state:
> >  #
> >  # Save the state of all devices to file. The RAM and the block devices
> 
> Eric, what's your take on this?
> 
> The general rule in QMP is "no ad hoc encoding of tuples in strings, use
> JSON objects instead".
> 
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index a85d847..60181c7 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -661,7 +661,36 @@ Example:
> >  <- { "return": {} }
> >  
> >  EQMP
> > -{
> > +
> > +    {
> > +        .name       = "migrate-incoming",
> > +        .args_type  = "uri:s",
> > +        .mhandler.cmd_new = qmp_marshal_input_migrate_incoming,
> > +    },
> > +
> > +SQMP
> > +migrate-incoming
> > +----------------
> > +
> > +Continue an incoming migration
> > +
> > +Arguments:
> > +
> > +- "uri": Source/listening URI (json-string)
> > +
> > +Example:
> > +
> > +-> { "execute": "migrate-incoming", "arguments": { "uri": "tcp::4446" } }
> > +<- { "return": {} }
> > +
> > +Notes:
> > +
> > +(1) QEMU must be started with -incoming defer to allow migrate-incoming to
> > +    be used
> > +(2) The uri format is the same as to -incoming
> > +
> > +EQMP
> > +    {
> >          .name       = "migrate-set-cache-size",
> >          .args_type  = "value:o",
> >          .mhandler.cmd_new = qmp_marshal_input_migrate_set_cache_size,
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 3/3] Document -incoming options

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Document the various URI formats for -incoming, the previous
> > manpage and help text was wrong (out of date?)
> 
> Thanks a lot for updating the docs.
> 
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > ---
> >  qemu-options.hx | 29 ++++++++++++++++++++++++++---
> >  1 file changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 85ca3ad..6d6d2a8 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -3169,12 +3169,35 @@ Set TB size.
> >  ETEXI
> >  
> >  DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
> > -    "-incoming p     prepare for incoming migration, listen on port p\n",
> > +    "-incoming uri   prepare for incoming migration, specifying source:\n" \
> > +    "                exec:command    Execute 'command' use the stdout as\n" \
> > +    "                                the migration stream\n" \
> > +    "                fd:num          listen on the given fd\n" \
> > +    "                defer           wait for the URI to be specified by\n" \
> > +    "                                the monitor (migrate_incoming)\n" \
> > +    "                rdma:addr:port  Listen on RDMA port on given address\n" \
> > +    "                tcp:addr:port   listen on TCP port (optional address)\n" \
> > +    "                unix:path       listen on the UNIX socket 'path'\n", \
> >      QEMU_ARCH_ALL)
> >  STEXI
> > -@item -incoming @var{port}
> > +@item -incoming @var{uri}
> >  @findex -incoming
> > -Prepare for incoming migration, listen on @var{port}.
> > +Prepare for incoming migration, specifying the source of the migration stream
> > +@table @option
> > +@item exec:@var{command}
> > +Execute 'command' and use the stdout as the migration stream.
> 
> Suggest:
> 
>     Execute 'command' and use its standard output as migration stream
>     source.

Yes OK, can do that.

> > +@item fd:@var{num}
> > +listen on the given fd
> 
> Well, we're not listening in usual the sense of listen(2).  Suggest
> 
>     Use file descriptor @var{num}.

OK.

> > +@item defer
> > +wait for the URI to be specified by the monitor (migrate_incoming)
> 
> "Wait", since the other items start with a capital letter.
> 
> > +@item rdma:@var{addr}:@var{port}
> > +Listen on RDMA port on given address
> 
> Let's call the thing between the colons "host" instead of "addr".  The
> address consists of host and port.

Yes I thought about doing that, but it really isn't a host, it's the
identifier for the network cards you're going to listen on; saying
'host' suggests thats that you need to specify the source host of the
destination which is certainly wrong.

> rdma_start_incoming_migration() passes everything after rdma: to
> qemu_rdma_data_init(), which parses it with inet_parse(), then uses only
> host and port.  The other members of InetSocketAddress are silently
> ignored.  Wonderful.

Yes, there are also some weird dependencies in the RDMA code on
IPv6 availability I've not quite figured out yet.

> qemu_rdma_data_init() errors out if you omit host.
> 
> inet_parse() accepts one of
> 
>     :PORT
>     [V6ADDR]:PORT
>     V4ADDR:PORT
>     HOST:PORT
> 
> followed by an options string.  The options string isn't really parsed,
> instead substrings matching one of these patterns are recognized:
> 
>     ,to=PORT
>     ,ipv4
>     ,ipv6
> 
> Note: you can specify ipv4,ipv6 or combine V6ADDR with ipv4, or V4ADDR
> with ipv6 to get both flags set in InetSocketAddress.
> 
> > +@item tcp:@var{addr}:@var{port}[,ipv4][,ipv6][,to=to]
> 
> [@var{host}] instead of @var{addr}, and [,to=@var{port}] instead of
> [,to=to], please.

OK.

> > +Listen on TCP port @var{port} (optional @var{addr} to specify address to listen on).
> > +The options ,ipv4, ipv6 and ,to are used in the same manner as chardev TCP options.
> 
> The reference to chardev options makes sense, but the reader needs to
> make the connection from "chardev TCP options" to "-chardev socket"
> himself.  Maybe "in the same manner as in -chardev socket"?

Yep OK.

> > +@item unix:@var{path}
> > +listen on the UNIX socket @var{path}
> > +@end table
> >  ETEXI
> >  
> >  DEF("nodefaults", 0, QEMU_OPTION_nodefaults, \
> 
> Don't get fooled by my many comments, this series is good stuff!
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK