[2/6] spice: client migration.

Hi,

>> I like client_migrate_info and it fits both spice+vnc naming too.
>>
>> Given that vnc just needs hostname and port (which are present
>> already) and the arguments not used by vnc are optional all we need
>> to do is rename the command and add a "protocol" argument similar to
>> "set_password", correct?
>
> Yeah, that sounds sufficient to me.

Quick incremental patch attached.  Became a bit larger than initially 
expected due to some code reorganization (move out of ui/spice-core.c) 
needed.

comments?

cheers,
   Gerd

On Mon, Jan 10, 2011 at 05:37:18PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >>I like client_migrate_info and it fits both spice+vnc naming too.
> >>
> >>Given that vnc just needs hostname and port (which are present
> >>already) and the arguments not used by vnc are optional all we need
> >>to do is rename the command and add a "protocol" argument similar to
> >>"set_password", correct?
> >
> >Yeah, that sounds sufficient to me.
> 
> Quick incremental patch attached.  Became a bit larger than
> initially expected due to some code reorganization (move out of
> ui/spice-core.c) needed.
> 
> comments?

Couldn't we just apply the migration info to all connected clients?
I mean, it doesn't make sense to have a connection open that you don't
want to migrate, and if we do want that we could always add that as
an optional last argument. i.e.

client_migrate_info <host> <port> <sport> <cert-subject> [<connection>]

(unrelated: are we assuming the same ca for both hosts? that isn't totally
obvious)

If you omit connection, we connect to all.

This way the user doesn't have to say spice/vnc again after already having
set it once at command line.

The way arguments work in qemu right now adding an optional argument isn't
that easy iirc, but I suggest just starting with "migrate all" functionality.

> 
> cheers,
>   Gerd

> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e6d8f36..05b777b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -815,24 +815,21 @@ ETEXI
>      },
>  
>  STEXI
> -@item spice_migrate_info @var{hostname} @var{port} @var{tls-port} @var{cert-subject}
> -@findex spice_migrate_info
> -Set the spice connection info for the migration target.  The spice
> -server will ask the spice client to automatically reconnect using the
> -new parameters (if specified) once the vm migration finished
> -successfully.
> +@item client_migrate_info @var{protocol} @var{hostname} @var{port} @var{tls-port} @var{cert-subject}
> +@findex client_migrate_info
> +Set the spice/vnc connection info for the migration target.  The spice/vnc
> +server will ask the spice/vnc client to automatically reconnect using the
> +new parameters (if specified) once the vm migration finished successfully.
>  ETEXI
>  
> -#if defined(CONFIG_SPICE)
>      {
> -        .name       = "spice_migrate_info",
> -        .args_type  = "hostname:s,port:i?,tls-port:i?,cert-subject:s?",
> -        .params     = "hostname port tls-port cert-subject",
> -        .help       = "send migration info to spice client",
> +        .name       = "client_migrate_info",
> +        .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
> +        .params     = "protocol hostname port tls-port cert-subject",
> +        .help       = "send migration info to spice/vnc client",
>          .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = mon_spice_migrate,
> +        .mhandler.cmd_new = client_migrate_info,
>      },
> -#endif
>  
>  STEXI
>  @item snapshot_blkdev
> diff --git a/monitor.c b/monitor.c
> index 038d532..6f5ee14 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1173,6 +1173,33 @@ static int expire_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      return -1;
>  }
>  
> +static int client_migrate_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *protocol = qdict_get_str(qdict, "protocol");
> +    const char *hostname = qdict_get_str(qdict, "hostname");
> +    const char *subject  = qdict_get_try_str(qdict, "cert-subject");
> +    int port             = qdict_get_try_int(qdict, "port", -1);
> +    int tls_port         = qdict_get_try_int(qdict, "tls-port", -1);
> +    int ret;
> +
> +    if (strcmp(protocol, "spice") == 0) {
> +        if (!using_spice) {
> +            qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
> +            return -1;
> +        }
> +
> +        ret = qemu_spice_migrate_info(hostname, port, tls_port, subject);
> +        if (ret != 0) {
> +            qerror_report(QERR_UNDEFINED_ERROR);
> +            return -1;
> +        }
> +        return 0;
> +    }
> +
> +    qerror_report(QERR_INVALID_PARAMETER, "protocol");
> +    return -1;
> +}
> +
>  static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>      vga_hw_screen_dump(qdict_get_str(qdict, "filename"));
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 24ada04..2ed8f44 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -503,39 +503,39 @@ EQMP
>      },
>  
>  SQMP
> -spice_migrate_info
> +client_migrate_info
>  ------------------
>  
> -Set the spice connection info for the migration target.  The spice
> -server will ask the spice client to automatically reconnect using the
> -new parameters (if specified) once the vm migration finished
> -successfully.
> +Set the spice/vnc connection info for the migration target.  The spice/vnc
> +server will ask the spice/vnc client to automatically reconnect using the
> +new parameters (if specified) once the vm migration finished successfully.
>  
>  Arguments:
>  
> +- "protocol":     protocol: "spice" or "vnc" (json-string)
>  - "hostname":     migration target hostname (json-string)
> -- "port":         spice tcp port for plaintext channels (json-int, optional)
> +- "port":         spice/vnc tcp port for plaintext channels (json-int, optional)
>  - "tls-port":     spice tcp port for tls-secured channels (json-int, optional)
>  - "cert-subject": server certificate subject (json-string, optional)
>  
>  Example:
>  
> --> { "execute": "spice_migrate_info",
> -     "arguments": { "hostname": "virt42.lab.kraxel.org", "port": 1234 } }
> +-> { "execute": "client_migrate_info",
> +     "arguments": { "protocol": "spice",
> +                    "hostname": "virt42.lab.kraxel.org",
> +                    "port": 1234 } }
>  <- { "return": {} }
>  
>  EQMP
>  
> -#if defined(CONFIG_SPICE)
>      {
> -        .name       = "spice_migrate_info",
> -        .args_type  = "hostname:s,port:i?,tls-port:i?,cert-subject:s?",
> -        .params     = "hostname port tls-port cert-subject",
> -        .help       = "send migration info to spice client",
> +        .name       = "client_migrate_info",
> +        .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
> +        .params     = "protocol hostname port tls-port cert-subject",
> +        .help       = "send migration info to spice/vnc client",
>          .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = mon_spice_migrate,
> +        .mhandler.cmd_new = client_migrate_info,
>      },
> -#endif
>  
>  SQMP
>  migrate_set_speed
> diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
> index f234c4d..78df3b4 100644
> --- a/ui/qemu-spice.h
> +++ b/ui/qemu-spice.h
> @@ -36,10 +36,11 @@ int qemu_spice_add_interface(SpiceBaseInstance *sin);
>  int qemu_spice_set_passwd(const char *passwd,
>                            bool fail_if_connected, bool disconnect_if_connected);
>  int qemu_spice_set_pw_expire(time_t expires);
> +int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
> +                            const char *subject);
>  
>  void do_info_spice_print(Monitor *mon, const QObject *data);
>  void do_info_spice(Monitor *mon, QObject **ret_data);
> -int mon_spice_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
>  
> @@ -48,6 +49,7 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
>  #define using_spice 0
>  #define qemu_spice_set_passwd(_p, _f1, _f2) (-1)
>  #define qemu_spice_set_pw_expire(_e) (-1)
> +#define qemu_spice_migrate_info(_h, _p, _t, _s) (-1)
>  
>  #endif /* CONFIG_SPICE */
>  
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 95116cc..1aa1a5e 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -431,26 +431,11 @@ static void migration_state_notifier(Notifier *notifier)
>      }
>  }
>  
> -int mon_spice_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
> +                            const char *subject)
>  {
> -    const char *hostname = qdict_get_str(qdict, "hostname");
> -    const char *subject  = qdict_get_try_str(qdict, "cert-subject");
> -    int port             = qdict_get_try_int(qdict, "port", -1);
> -    int tls_port         = qdict_get_try_int(qdict, "tls-port", -1);
> -    int ret;
> -
> -    if (!spice_server) {
> -        qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
> -        return -1;
> -    }
> -
> -    ret = spice_server_migrate_info(spice_server, hostname,
> -                                    port, tls_port, subject);
> -    if (ret != 0) {
> -        qerror_report(QERR_UNDEFINED_ERROR);
> -        return -1;
> -    }
> -    return 0;
> +    return spice_server_migrate_info(spice_server, hostname,
> +                                     port, tls_port, subject);
>  }
>  
>  static int add_channel(const char *name, const char *value, void *opaque)

On Mon, Jan 10, 2011 at 09:26:12PM +0200, Alon Levy wrote:
> On Mon, Jan 10, 2011 at 05:37:18PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > >>I like client_migrate_info and it fits both spice+vnc naming too.
> > >>
> > >>Given that vnc just needs hostname and port (which are present
> > >>already) and the arguments not used by vnc are optional all we need
> > >>to do is rename the command and add a "protocol" argument similar to
> > >>"set_password", correct?
> > >
> > >Yeah, that sounds sufficient to me.
> > 
> > Quick incremental patch attached.  Became a bit larger than
> > initially expected due to some code reorganization (move out of
> > ui/spice-core.c) needed.
> > 
> > comments?
> 
> Couldn't we just apply the migration info to all connected clients?
> I mean, it doesn't make sense to have a connection open that you don't
> want to migrate, and if we do want that we could always add that as
> an optional last argument. i.e.
> 
> client_migrate_info <host> <port> <sport> <cert-subject> [<connection>]
> 
> (unrelated: are we assuming the same ca for both hosts? that isn't totally
> obvious)

That shouldn't matter either way. Whether there is the same CA or
different CAs, the key factor is that the client must have the CA
used in each host in its trusted set. Only once it has decided it
trusts the CA, does it go on to use the cert-subject data for the
next step of validation.

> If you omit connection, we connect to all.
> 
> This way the user doesn't have to say spice/vnc again after already having
> set it once at command line.

If you have both spice and VNC displays active, then you need to
specify different ports for each. You might also have them listening
on different IP addresses, and potentially using different certs
(though the latter is unlikely). So I think we do need to specify
this data separately for each network service that needs migration
support

Regards,
Daniel

Hi,

> This way the user doesn't have to say spice/vnc again after already having
> set it once at command line.

Doesn't fly.  You can activate *both* vnc and spice, and in that case 
management has to send two client_migrate_info commands, one for spice 
and one for vnc.  Also this is consistent with the set_password and 
expire_password commands which have protocol as first argument too.

cheers,
   Gerd