Re: [Qemu-devel] [PATCH 12/15] qapi: add change-vnc-listen (v2)

[Daniel P. Berrange]

On Mon, Sep 12, 2011 at 10:17:21AM +0100, Daniel P. Berrange wrote:
> On Fri, Sep 02, 2011 at 05:50:05PM -0300, Luiz Capitulino wrote:
> > On Fri,  2 Sep 2011 12:34:55 -0500
> > Anthony Liguori <address@hidden> wrote:
> > 
> > > New QMP only command to change the VNC server's listening address.
> > > 
> > > Signed-off-by: Anthony Liguori <address@hidden>
> > > ---
> > > v1 -> v2
> > >  - Enhanced docs (Luiz)
> > > ---
> > >  qapi-schema.json |   15 +++++++++++++++
> > >  qmp-commands.hx  |    8 ++++++++
> > >  qmp.c            |    7 +++++++
> > >  3 files changed, 30 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 350cf1c..0c6c9b8 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -109,3 +109,18 @@
> > >  #         string.  Existing clients are unaffected by executing this 
> > > command.
> > >  ##
> > >  { 'command': 'change-vnc-password', 'data': {'password': 'str'} }
> > > +
> > > +##
> > > +# @change-vnc-listen:
> > > +#
> > > +# Change the host that the VNC server listens on.
> > > +#
> > > +# @target:  the new server specification to listen on
> > > +#
> > > +# Since: 1.0
> > > +#
> > > +# Notes:  At this moment in time, the behavior of existing client 
> > > connections
> > > +#         when this command is executed is undefined.  The authentication
> > > +#         settings may change after executing this command.
> > 
> > It seems to completely disable authentication. At least when using
> > password auth. I'd be very clear about that.
> 
> That is really bad, since even if we have another command to set the
> authentication mode, this creates a designed-in race condition. To be
> securely race-free, we need to be able to set the desired auth mode
> first, and then change the listen address without it affecting auth.
> 
>     change-vnc-auth   tls
>     change-vnc-listen 123.2.3.5:5901

On closer inspection, I see that 'change-vnc-listen' just accepts the
full string with encoded options, that is used for the '-vnc' command
line. I thought that for QMP we going to make sure we didn't use any
encoded strings, and gave each option a dedicated parameter ?

eg instead of:

  { 'command': 'change-vnc-password', 'data': {'target': 'str'} }

Wouldn't we want something like:

  { 'command': 'change-vnc-password', 'data': {
        'listen': bool,    /* Whether to listen, or do a reverse connection */
        'address': 'str',
        'port': 'int',
        'password': 'string',
        'sasl': bool,
        'tls': bool,
        'x509': bool,
        'lossy': bool,
        'no-lock-key-sync': bool,
        ...
     }
   }

At which point we could also make  '-vnc' use qemu-config for its option
parsing ?

Or is your idea that we just do the more straightforward QMP command for
change-vnc-listen now, with the view that everything will be changed for
the future QEMU Object model rewrite ?

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|