On Tue, May 08, 2012 at 12:19:45PM -0600, Eric Blake wrote:
> On 05/08/2012 11:42 AM, Alon Levy wrote:
> > qemu's behavior in this case is to change the spice server behavior to
> > require secure connection to any channel not otherwise specified as
> > being in plaintext mode. libvirt doesn't currently allow requesting this
> > (via plaintext-channel=<channel name>).
> >
> > RHBZ: 819499
> >
> > Signed-off-by: Alon Levy <alevy@xxxxxxxxxx>
> > ---
> > docs/formatdomain.html.in | 3 +++
> > docs/schemas/domaincommon.rng | 9 +++++++++
> > src/conf/domain_conf.c | 20 ++++++++++++++++++++
> > src/conf/domain_conf.h | 1 +
> > src/qemu/qemu_command.c | 13 +++++++++++++
> > .../qemuxml2argv-graphics-spice.args | 2 +-
> > .../qemuxml2argv-graphics-spice.xml | 2 +-
> > 7 files changed, 48 insertions(+), 2 deletions(-)
>
> Definitely more code, but I think I like it better. In particular,
>
> >
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 0525577..c0268b2 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -2929,6 +2929,9 @@ qemu-kvm -net nic,model=? /dev/null
> > <span class="since">Since 0.9.3</span>
> > NB, this may not be supported by all hypervisors.
> > <span class="since">"spice" since 0.8.6</span>.
> > + The <code>defaultMode</code> attribute sets the default channel
> > + security policy, valid values are <code>secure</code>,
> > + <code>insecure</code> and the default <code>any</code>.
>
> I didn't realize that 'any' was different! Having a defaultMode call it
> out makes sense now, given this matrix:
>
> insecure any secure
> tls available plaintext tls tls
> tls disabled plaintext plaintext error
>
> And it also makes sense to the qemu command line - you can only specify
> plaintext or tls to lock a channel to a particular mode; if you omit the
> specification, then the mode defaults to the best available (according
> to whether tls is available).
Your table is correct and a good summary. But to be clear the "any"
value is not a valid value to put in qemu's command line, it's the
internal default state (actually it's oring of the two valid channel
modes, insecure and secure). But it behaves exactly like your table
explains, i.e. not setting tls-channel=default or
plaintext-channel=default results in the middle column, setting
tls-channel=default in the right, and plaintext-channel=default in the
left. You could have expanded the table because it's also possible to
start spice with only tls-port set, and no port, then you would get the
reverse of the right column:
insecure any secure
both plaintext tls tls
only clear plaintext plaintext error
only tls error plaintext tls
>
> Missing a <span>since</span> notation; I've added that.
>
> > @@ -12124,6 +12140,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
> > virBufferEscapeString(buf, " keymap='%s'",
> > def->data.spice.keymap);
> >
> > + if (def->data.spice.defaultMode != VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY)
> > + virBufferAsprintf(buf, " defaultMode='%s'",
> > + virDomainGraphicsSpiceChannelModeTypeToString(def->data.spice.defaultMode));
>
> This means we don't output the user's explicit defaultMode='any'. It's
> easy enough to fix by making the enum have four values instead of three
> (the fourth value, 'default', is value 0 and not valid in XML, as the
> placeholder for no attribute present, but otherwise behaves like 'any').
> Then again, you matched the style of the individual channels, so I
> won't bother changing it.
>
> > +++ b/src/conf/domain_conf.h
> > @@ -1233,6 +1233,7 @@ struct _virDomainGraphicsDef {
> > virDomainGraphicsAuthDef auth;
> > unsigned int autoport :1;
> > int channels[VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST];
> > + int defaultMode;
>
> I like to add a comment to the enum values that are valid in this field.
>
> ACK; I squashed this in, then pushed:
Looks good.
>
> diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
> index 816af41..1478832 100644
> --- i/docs/formatdomain.html.in
> +++ w/docs/formatdomain.html.in
> @@ -2931,7 +2931,11 @@ qemu-kvm -net nic,model=? /dev/null
> <span class="since">"spice" since 0.8.6</span>.
> The <code>defaultMode</code> attribute sets the default
> channel
> security policy, valid values are <code>secure</code>,
> - <code>insecure</code> and the default <code>any</code>.
> + <code>insecure</code> and the default <code>any</code>
> + (which is secure if possible, but falls back to insecure
> + rather than erroring out if no secure path is
> + available). <span class="since">"defaultMode" since
> + 0.9.12</span>.
> </p>
> <p>
> When SPICE has both a normal and TLS secured TCP port
> diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h
> index 895ddc4..00178e1 100644
> --- i/src/conf/domain_conf.h
> +++ w/src/conf/domain_conf.h
> @@ -1233,7 +1233,7 @@ struct _virDomainGraphicsDef {
> virDomainGraphicsAuthDef auth;
> unsigned int autoport :1;
> int channels[VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST];
> - int defaultMode;
> + int defaultMode; /* enum virDomainGraphicsSpiceChannelMode */
> int image;
> int jpeg;
> int zlib;
>
> --
> Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list