[libvirt] [PATCH 0/2] fix corner case of spice graphics channel mode settings
Pavel Hrdina (2): qemu: remove duplicated code for allocating spice ports qemu: check defaultMode for spice graphics independently src/qemu/qemu_driver.c | 45 ++---------------------------- src/qemu/qemu_process.c | 73 ++++++++++++++++++++++++++++--------------------- src/qemu/qemu_process.h | 5 ++++ 3 files changed, 49 insertions(+), 74 deletions(-) -- 2.0.5
We have two different places that needs to be updated while touching code for allocation spice ports. Add a bool option to 'qemuProcessSPICEAllocatePorts' function to switch between true and fake allocation so we can use this function also in qemu_driver to generate native domain definition. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 45 ++------------------------------------------- src/qemu/qemu_process.c | 42 ++++++++++++++++++++++++++---------------- src/qemu/qemu_process.h | 5 +++++ 3 files changed, 33 insertions(+), 59 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e282464..46439cb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6579,49 +6579,8 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, !graphics->data.vnc.socket && graphics->data.vnc.autoport) { graphics->data.vnc.port = 5900; } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { - size_t j; - bool needTLSPort = false; - bool needPort = false; - int defaultMode = graphics->data.spice.defaultMode; - - if (graphics->data.spice.autoport) { - /* check if tlsPort or port need allocation */ - for (j = 0; j < VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST; j++) { - switch (graphics->data.spice.channels[j]) { - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: - needTLSPort = true; - break; - - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: - needPort = true; - break; - - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: - switch (defaultMode) { - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: - needTLSPort = true; - break; - - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: - needPort = true; - break; - - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: - if (cfg->spiceTLS) - needTLSPort = true; - needPort = true; - break; - } - break; - } - } - } - - if (needPort || graphics->data.spice.port == -1) - graphics->data.spice.port = 5901; - - if (needTLSPort || graphics->data.spice.tlsPort == -1) - graphics->data.spice.tlsPort = 5902; + ignore_value(qemuProcessSPICEAllocatePorts(driver, cfg, + graphics, false)); } } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 515402e..aeb479c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4019,10 +4019,11 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver, return 0; } -static int +int qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, - virDomainGraphicsDefPtr graphics) + virDomainGraphicsDefPtr graphics, + bool allocate) { unsigned short port = 0; unsigned short tlsPort; @@ -4066,30 +4067,39 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, } if (needPort || graphics->data.spice.port == -1) { - if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0) - goto error; + if (allocate) { + if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0) + goto error; - graphics->data.spice.port = port; + graphics->data.spice.port = port; + } else { + graphics->data.spice.port = 5901; + } } if (needTLSPort || graphics->data.spice.tlsPort == -1) { - if (!cfg->spiceTLS) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Auto allocation of spice TLS port requested " - "but spice TLS is disabled in qemu.conf")); - goto error; - } + if (allocate) { + if (!cfg->spiceTLS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Auto allocation of spice TLS port requested " + "but spice TLS is disabled in qemu.conf")); + goto error; + } - if (virPortAllocatorAcquire(driver->remotePorts, &tlsPort) < 0) - goto error; + if (virPortAllocatorAcquire(driver->remotePorts, &tlsPort) < 0) + goto error; - graphics->data.spice.tlsPort = tlsPort; + graphics->data.spice.tlsPort = tlsPort; + } else { + graphics->data.spice.tlsPort = 5902; + } } return 0; error: - virPortAllocatorRelease(driver->remotePorts, port); + if (allocate) + virPortAllocatorRelease(driver->remotePorts, port); return -1; } @@ -4573,7 +4583,7 @@ int qemuProcessStart(virConnectPtr conn, if (qemuProcessVNCAllocatePorts(driver, graphics) < 0) goto cleanup; } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { - if (qemuProcessSPICEAllocatePorts(driver, cfg, graphics) < 0) + if (qemuProcessSPICEAllocatePorts(driver, cfg, graphics, true) < 0) goto cleanup; } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2e1d393..5c70803 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -107,4 +107,9 @@ int qemuProcessReadLog(int fd, char *buf, int buflen, int off, bool skipchar); int qemuProcessSetSchedParams(int id, pid_t pid, size_t nsp, virDomainThreadSchedParamPtr sp); +int qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, + virQEMUDriverConfigPtr cfg, + virDomainGraphicsDefPtr graphics, + bool allocate); + #endif /* __QEMU_PROCESS_H__ */ -- 2.0.5
On Fri, Feb 27, 2015 at 03:35:26PM +0100, Pavel Hrdina wrote:
We have two different places that needs to be updated while touching code for allocation spice ports. Add a bool option to 'qemuProcessSPICEAllocatePorts' function to switch between true and fake allocation so we can use this function also in qemu_driver to generate native domain definition.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 45 ++------------------------------------------- src/qemu/qemu_process.c | 42 ++++++++++++++++++++++++++---------------- src/qemu/qemu_process.h | 5 +++++ 3 files changed, 33 insertions(+), 59 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e282464..46439cb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6579,49 +6579,8 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, !graphics->data.vnc.socket && graphics->data.vnc.autoport) { graphics->data.vnc.port = 5900; } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { - size_t j; - bool needTLSPort = false; - bool needPort = false; - int defaultMode = graphics->data.spice.defaultMode; - - if (graphics->data.spice.autoport) { - /* check if tlsPort or port need allocation */ - for (j = 0; j < VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST; j++) { - switch (graphics->data.spice.channels[j]) { - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: - needTLSPort = true; - break; - - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: - needPort = true; - break; - - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: - switch (defaultMode) { - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: - needTLSPort = true; - break; - - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: - needPort = true; - break; - - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: - if (cfg->spiceTLS) - needTLSPort = true; - needPort = true; - break; - } - break; - } - } - } - - if (needPort || graphics->data.spice.port == -1) - graphics->data.spice.port = 5901; - - if (needTLSPort || graphics->data.spice.tlsPort == -1) - graphics->data.spice.tlsPort = 5902; + ignore_value(qemuProcessSPICEAllocatePorts(driver, cfg, + graphics, false));
Don't ignore any errors (even though currently there won't be any), just jump to clean-up for return value < 0.
} }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 515402e..aeb479c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4019,10 +4019,11 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver, return 0; }
-static int +int qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, - virDomainGraphicsDefPtr graphics) + virDomainGraphicsDefPtr graphics, + bool allocate) { unsigned short port = 0; unsigned short tlsPort; @@ -4066,30 +4067,39 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, }
if (needPort || graphics->data.spice.port == -1) { - if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0) - goto error; + if (allocate) { + if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0) + goto error;
- graphics->data.spice.port = port; + graphics->data.spice.port = port; + } else { + graphics->data.spice.port = 5901; + } }
if (needTLSPort || graphics->data.spice.tlsPort == -1) { - if (!cfg->spiceTLS) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Auto allocation of spice TLS port requested " - "but spice TLS is disabled in qemu.conf")); - goto error; - } + if (allocate) {
I'd rather use: if (!allocate) { if (needPort || graphics->data.spice.port == -1) graphics->data.spice.port = 5901; if (needTlsPort || graphics->data.spice.tlsPort == -1) graphics->data.spice.tlsPort = 5902; return 0; } /* and here deal with the allocations */ but what you've done is fine, too. ACK with or without that changed.
Instead of checking defaultMode for every channel that has no mode configured, test it only once outside of channel loop. This fixes a bug that in case all possible channels are fore example set to insecure, but defaultMode is set to secure, we wouldn't auto-generate TLS port. This results in failure while starting a guest. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1143832 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_process.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index aeb479c..bd63fd7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4046,24 +4046,25 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, break; case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: - switch (defaultMode) { - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: - needTLSPort = true; - break; - - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: - needPort = true; - break; - - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: - if (cfg->spiceTLS) - needTLSPort = true; - needPort = true; - break; - } + /* default mode will be used */ break; } } + switch (defaultMode) { + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: + needTLSPort = true; + break; + + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: + needPort = true; + break; + + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: + if (cfg->spiceTLS) + needTLSPort = true; + needPort = true; + break; + } } if (needPort || graphics->data.spice.port == -1) { -- 2.0.5
On Fri, Feb 27, 2015 at 03:35:27PM +0100, Pavel Hrdina wrote:
Instead of checking defaultMode for every channel that has no mode configured, test it only once outside of channel loop. This fixes a bug that in case all possible channels are fore example set to insecure, but defaultMode is set to secure, we wouldn't auto-generate TLS port. This results in failure while starting a guest.
Also covers all possible future channels. ACK.
participants (2)
-
Martin Kletzander -
Pavel Hrdina