Re: [libvirt] [PATCH 02/17] qemu_process: move listen code out of qemuProcessSetupGraphics
On Fri, May 06, 2016 at 02:10:10PM +0200, Peter Krempa wrote:
On Thu, May 05, 2016 at 18:20:21 +0200, Pavel Hrdina wrote:
> Move adding the config listen type=address if there is none in
> qemuProcessPrepareDomain and move check for multiple listens to
> qemuProcessStartValidate.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/qemu/qemu_process.c | 75 ++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 55 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 348b392..17fd566 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5138,6 +5143,35 @@ qemuProcessPrepareDomain(virConnectPtr conn,
> if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0)
> goto cleanup;
>
> + /* Fill in run-time values for graphics devices. */
> + for (i = 0; i < vm->def->ngraphics; i++) {
> + virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
> + char *listenAddr = NULL;
> +
> + switch (graphics->type) {
> + case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
> + listenAddr = cfg->vncListen;
> + case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> + if (!listenAddr)
This fallthrough case with checking whether this was set is weird.
Looking at the caps code it looks like its guaranteed that both
vncListen and spiceListen are always allocated, but still it looks like
both types should either have individual calls to
virDomainGraphicsListenAppendAddress or the call should happen after the
switch so that we don't obscure it using the fallthrough case.
Right, it can be easily updated that the listenAddr is set only for vnc or spice
and the virDomainGraphicsListenAppendAddress could be moved out of the switch
with this condition: if (graphics->nListens == 0 && listeAddr). Do you
require
v2 for this?
> + listenAddr = cfg->spiceListen;
> +
> + if (graphics->nListens == 0) {
> + if (virDomainGraphicsListenAppendAddress(graphics,
> + listenAddr) < 0)
> + goto cleanup;
> +
> + graphics->listens[0].fromConfig = true;
> + }
> + break;
> +
> + case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
> + case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
> + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
> + case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
> + break;
> + }
> + }
> +
> /* "volume" type disk's source must be translated before
> * cgroup and security setting.
> */