Re: [Qemu-devel] [PATCH v2 09/10] monitor: add query-vnc2 command

[Daniel P. Berrange]

On Thu, Dec 11, 2014 at 10:07:24AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > +# @auth: The current authentication type used by the server
> > > +#        'none' if no authentication is being used
> > > +#        'vnc' if VNC authentication is being used
> > > +#        'vencrypt+plain' if VEncrypt is used with plain text 
> > > authentication
> > > +#        'vencrypt+tls+none' if VEncrypt is used with TLS and no 
> > > authentication
> > > +#        'vencrypt+tls+vnc' if VEncrypt is used with TLS and VNC 
> > > authentication
> > > +#        'vencrypt+tls+plain' if VEncrypt is used with TLS and plain 
> > > text auth
> > > +#        'vencrypt+x509+none' if VEncrypt is used with x509 and no auth
> > > +#        'vencrypt+x509+vnc' if VEncrypt is used with x509 and VNC auth
> > > +#        'vencrypt+x509+plain' if VEncrypt is used with x509 and plain 
> > > text auth
> > > +#        'vencrypt+tls+sasl' if VEncrypt is used with TLS and SASL auth
> > > +#        'vencrypt+x509+sasl' if VEncrypt is used with x509 and SASL auth
> > 
> > This feels like an open-coded string that should instead be an array of
> > enum values.
> 
> This is just copyed over from the old query-vnc command.
> 
> We can try to do that in a cleaner way for query-vnc2.  That implies
> libvirt needs to be able to handle both formats though.

It isn't a big deal if the format changes. Libvirt doesn't use the
auth value for any functional purpose. It just passes the string
auth scheme name to the event callback that fires whenever a user
completes authentication. So if the fields were split, libvirt
would just concatenate them again.


> 
> >   That is,
> > 
> > { 'enum': 'VncAuth', 'data', [ 'none', 'vnc', 'vencrypt', 'plain',
> > 'tls', 'x509' ] }
> 
> > ... 'auth': ['VcnAuth']
> 
> Looked at the source, figured the list grew meanwhile.
> 
> Do we need 'none' here?  When we have a list anyway we can have an empty
> list instead.

Yes, I think "none" should be explicit, because you'll need it in
combination with vencrypt too - eg vencrypt-tls-none where you can
infer it from an empty list.

Regards,
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 :|