[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 2/2] domain_conf: add "default" to list of valid spice channels



On Tue, May 08, 2012 at 10:10:34PM +0300, Alon Levy wrote:
> 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 redhat com>
> > > ---
> > >  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
This would actually be:
both                   plaintext  whichever-the-client-first-connects-to
                                             tls

> only clear           plaintext  plaintext  error
> only tls             error      plaintext  tls
Oops, should read:
only tls             error      tls        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 redhat com    +1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> > 
> 
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]