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

On 05/19/2016 07:35 AM, Pavel Hrdina wrote: > This prepares the code for other listen types. > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > 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.

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