[libvirt] [PATCHv2] qemu: Do fake auto-allocation of ports when generating native command
When attempting to generate the native command line from an XML file that uses graphics port auto allocation, the generated commandline wouldn't be valid. This patch adds fake autoallocation of ports as done when starting the actual machine. --- Notes: Version 2: - rebased after context-conflict with 11fc1beab6e018a88182f80056d35217c150b3de src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9492850..45fcf05 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5274,6 +5274,58 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) goto cleanup; + /* do fake auto-alloc of graphics ports, if such config is used */ + for (i = 0 ; i < def->ngraphics; ++i) { + virDomainGraphicsDefPtr graphics = def->graphics[i]; + if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && + !graphics->data.vnc.socket && graphics->data.vnc.autoport) { + graphics->data.vnc.port = 5900; + } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + int 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: + 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; + } + } + if (!(cmd = qemuBuildCommandLine(conn, driver, def, &monConfig, monitor_json, qemuCaps, NULL, -1, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP))) -- 1.8.2.1
On 05/02/13 11:16, Peter Krempa wrote:
When attempting to generate the native command line from an XML file that uses graphics port auto allocation, the generated commandline wouldn't be valid.
This patch adds fake autoallocation of ports as done when starting the actual machine. ---
Notes: Version 2: - rebased after context-conflict with 11fc1beab6e018a88182f80056d35217c150b3de
src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)
Ping? Could somebody please have a look? Peter
On 05/02/2013 03:16 AM, Peter Krempa wrote:
When attempting to generate the native command line from an XML file that uses graphics port auto allocation, the generated commandline wouldn't be valid.
This patch adds fake autoallocation of ports as done when starting the actual machine. ---
Notes: Version 2: - rebased after context-conflict with 11fc1beab6e018a88182f80056d35217c150b3de
src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)
ACK.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9492850..45fcf05 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5274,6 +5274,58 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) goto cleanup;
+ /* do fake auto-alloc of graphics ports, if such config is used */ + for (i = 0 ; i < def->ngraphics; ++i) { + virDomainGraphicsDefPtr graphics = def->graphics[i]; + if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && + !graphics->data.vnc.socket && graphics->data.vnc.autoport) { + graphics->data.vnc.port = 5900;
I don't know if using QEMU_REMOTE_PORT_MIN instead of hard-coding the magic number is any better, but we already discussed that for purposes of command-line generation, ANY port number will do, so it doesn't really invalidate your patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/06/13 19:09, Eric Blake wrote:
On 05/02/2013 03:16 AM, Peter Krempa wrote:
When attempting to generate the native command line from an XML file that uses graphics port auto allocation, the generated commandline wouldn't be valid.
This patch adds fake autoallocation of ports as done when starting the actual machine. ---
Notes: Version 2: - rebased after context-conflict with 11fc1beab6e018a88182f80056d35217c150b3de
src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)
ACK.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9492850..45fcf05 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5274,6 +5274,58 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) goto cleanup;
+ /* do fake auto-alloc of graphics ports, if such config is used */ + for (i = 0 ; i < def->ngraphics; ++i) { + virDomainGraphicsDefPtr graphics = def->graphics[i]; + if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && + !graphics->data.vnc.socket && graphics->data.vnc.autoport) { + graphics->data.vnc.port = 5900;
I don't know if using QEMU_REMOTE_PORT_MIN instead of hard-coding the magic number is any better, but we already discussed that for purposes of command-line generation, ANY port number will do, so it doesn't really invalidate your patch.
Thanks for the reviews. This patch is now pushed. As this is a fix that we generate a valid commandline for qemu, when somebody tries to use the domxml-to-native command/API I don't think it's necessary to spend more time on it. I was considering if it's even worth doing this at all. I decided that we should do it this way, as nobody can expect that autoallocation should work outside of libvirt's control. Peter
On 02.05.2013 11:16, Peter Krempa wrote:
When attempting to generate the native command line from an XML file that uses graphics port auto allocation, the generated commandline wouldn't be valid.
This patch adds fake autoallocation of ports as done when starting the actual machine. ---
Notes: Version 2: - rebased after context-conflict with 11fc1beab6e018a88182f80056d35217c150b3de
src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9492850..45fcf05 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5274,6 +5274,58 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) goto cleanup;
+ /* do fake auto-alloc of graphics ports, if such config is used */ + for (i = 0 ; i < def->ngraphics; ++i) {
I think we prefer postfix notation in for().
+ virDomainGraphicsDefPtr graphics = def->graphics[i]; + if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && + !graphics->data.vnc.socket && graphics->data.vnc.autoport) { + graphics->data.vnc.port = 5900; + } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + int 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: + needTLSPort = true; + needPort = true; + break; + } + break; + } + } + }
This chunk is copied from qemu_process.c from qemuProcessSPICEAllocatePorts(). Shall we somehow create a function helper for this?
+ + if (needPort || graphics->data.spice.port == -1) + graphics->data.spice.port = 5901; + + if (needTLSPort || graphics->data.spice.tlsPort == -1) + graphics->data.spice.tlsPort = 5902; + } + } + if (!(cmd = qemuBuildCommandLine(conn, driver, def, &monConfig, monitor_json, qemuCaps, NULL, -1, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP)))
If we don't need a helper (and hence v3) I can live with this as is. ACK. Michal
participants (3)
-
Eric Blake -
Michal Privoznik -
Peter Krempa