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@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