[libvirt] [PATCH v4 08/14] qemu_command: refactor spice channel code

[Pavel Hrdina]

On Thu, May 19, 2016 at 05:37:43PM -0400, Cole Robinson wrote:
> On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
> > This prepares the code for other listen types.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > ---
> >  src/qemu/qemu_command.c | 57 ++++++++++++++++++++++---------------------------
> >  1 file changed, 26 insertions(+), 31 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index ee43e21..e401ac5 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -7500,10 +7500,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> >  {
> >      virBuffer opt = VIR_BUFFER_INITIALIZER;
> >      virDomainGraphicsListenDefPtr glisten = NULL;
> > -    int defaultMode = graphics->data.spice.defaultMode;
> >      int port = graphics->data.spice.port;
> >      int tlsPort = graphics->data.spice.tlsPort;
> >      size_t i;
> > +    bool hasSecure = false;
> > +    bool hasInsecure = false;
> >  
> >      if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE)) {
> >          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > @@ -7513,8 +7514,10 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> >  
> >      glisten = virDomainGraphicsGetListen(graphics, 0);
> >  
> > -    if (port > 0)
> > +    if (port > 0) {
> >          virBufferAsprintf(&opt, "port=%u,", port);
> > +        hasInsecure = true;
> > +    }
> >  
> >      if (tlsPort > 0) {
> >          if (!cfg->spiceTLS) {
> > @@ -7524,6 +7527,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> >              goto error;
> >          }
> >          virBufferAsprintf(&opt, "tls-port=%u,", tlsPort);
> > +        hasSecure = true;
> >      }
> >  
> >      if (port > 0 || tlsPort > 0) {
> > @@ -7561,17 +7565,30 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> >          !cfg->spicePassword)
> >          virBufferAddLit(&opt, "disable-ticketing,");
> >  
> > -    if (tlsPort > 0)
> > +    if (hasSecure)
> >          virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir);
> >  
> > -    switch (defaultMode) {
> > +    switch (graphics->data.spice.defaultMode) {
> >      case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
> > +        if (!hasSecure) {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("spice defaultMode secure requested in XML "
> > +                             "configuration, but TLS is not available"));
> > +            goto error;
> > +        }
> >          virBufferAddLit(&opt, "tls-channel=default,");
> >          break;
> >      case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE:
> > +        if (!hasInsecure) {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("spice defaultMode insecure requested in XML "
> > +                             "configuration, but plaintext is not available"));
> > +            goto error;
> > +        }
> >          virBufferAddLit(&opt, "plaintext-channel=default,");
> >          break;
> >      case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY:
> > +    case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_LAST:
> >          /* nothing */
> >          break;
> >      }
> > @@ -7579,10 +7596,10 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> >      for (i = 0; i < VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST; i++) {
> >          switch (graphics->data.spice.channels[i]) {
> >          case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
> > -            if (tlsPort <= 0) {
> > +            if (!hasSecure) {
> >                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > -                               _("spice secure channels set in XML configuration, "
> > -                                 "but TLS port is not provided"));
> > +                               _("spice secure channels set in XML "
> > +                                 "configuration, but TLS is not available"));
> >                  goto error;
> >              }
> 
> I kinda prefer the original messages mentioning the lack of port as the
> culprit. So maybe plaintext port and TLS port? If I saw the 'plaintext' error
> as is I know I'd be confused and go digging into the code to figure out what
> was triggering it.

I've just come up with a little improvement, instead of port I'll use
connection.  It cannot refer to a port, because those error messages could be
printed also for listen type socket or none, where we don't have any ports.

> 
> ACK otherwise. IMO these first 8 patches are worth pushing and we work out any
> additional stuff like the type='network' weirdness, and additional test cases,
> on top of these cleanups
 
Thanks, I've fixed some of the nits you've pointed out and I'll push those
patches.

Pavel