[libvirt] [Charles_Duffy@messageone.com: [PATCH] autoport overrides actual VNC port number in dump-xml]
Forwarding message that was sent to wrong list... ----- Forwarded message from Charles Duffy <Charles_Duffy@messageone.com> -----
To: kvm@vger.kernel.org From: Charles Duffy <Charles_Duffy@messageone.com> Subject: [PATCH] autoport overrides actual VNC port number in dump-xml Date: Wed, 30 Jul 2008 00:32:57 -0500
Per subject; if autoport is in use for a host, the current virDomainGraphicsDefFormat code always emits "port=-1", even if a port is assigned to the host; this leaves no way for a client to find the VNC port assigned to the host in question.
--------------090103050500010408080902 Content-Type: text/x-diff; name="libvirt-fix-vnc-port-output.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="libvirt-fix-vnc-port-output.patch"
diff --git a/src/domain_conf.c b/src/domain_conf.c index d629093..ece471e 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -2431,11 +2431,11 @@ virDomainGraphicsDefFormat(virConnectPtr conn,
switch (def->type) { case VIR_DOMAIN_GRAPHICS_TYPE_VNC: - if (def->data.vnc.autoport) - virBufferAddLit(buf, " port='-1'"); - else if (def->data.vnc.port) + if (def->data.vnc.port) virBufferVSprintf(buf, " port='%d'", def->data.vnc.port); + else if (def->data.vnc.autoport) + virBufferAddLit(buf, " port='-1'");
virBufferVSprintf(buf, " autoport='%s'", def->data.vnc.autoport ? "yes" : "no");
--------------090103050500010408080902--
-- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
----- End forwarded message ----- -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Jul 30, 2008 at 10:17:59AM +0100, Daniel P. Berrange wrote:
Forwarding message that was sent to wrong list...
Hum, that sounds right, the logic got twisted, the presence of a defined port should otherride the autoport settings, right ? Daniel
case VIR_DOMAIN_GRAPHICS_TYPE_VNC: - if (def->data.vnc.autoport) - virBufferAddLit(buf, " port='-1'"); - else if (def->data.vnc.port) + if (def->data.vnc.port) virBufferVSprintf(buf, " port='%d'", def->data.vnc.port); + else if (def->data.vnc.autoport) + virBufferAddLit(buf, " port='-1'");
-- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Jul 30, 2008 at 05:33:22AM -0400, Daniel Veillard wrote:
On Wed, Jul 30, 2008 at 10:17:59AM +0100, Daniel P. Berrange wrote:
Forwarding message that was sent to wrong list...
Hum, that sounds right, the logic got twisted, the presence of a defined port should otherride the autoport settings, right ?
Yes, this logic was coded badly by me. What I was trying to address here was that the declared port should be reset to -1 for inactive domains, when autoport is yes. I ended up always setting it to -1. So instead of this: if (def->data.vnc.autoport) virBufferAddLit(buf, " port='-1'"); else if (def->data.vnc.port) virBufferVSprintf(buf, " port='%d'", def->data.vnc.port); What I think we need is this: if (def->data.vnc.port && (!def->data.vnc.autoport || def->id != -1)) virBufferVSprintf(buf, " port='%d'", def->data.vnc.port); else if (def->data.vnc.autiport) virBufferAddLit(buf, " port='-1'"); So the explicit port is output in XML if autoport is not set, or if the domain is running. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Jul 30, 2008 at 10:43:35AM +0100, Daniel P. Berrange wrote:
On Wed, Jul 30, 2008 at 05:33:22AM -0400, Daniel Veillard wrote:
On Wed, Jul 30, 2008 at 10:17:59AM +0100, Daniel P. Berrange wrote:
Forwarding message that was sent to wrong list...
Hum, that sounds right, the logic got twisted, the presence of a defined port should otherride the autoport settings, right ?
Yes, this logic was coded badly by me. What I was trying to address here was that the declared port should be reset to -1 for inactive domains, when autoport is yes. I ended up always setting it to -1.
So instead of this:
if (def->data.vnc.autoport) virBufferAddLit(buf, " port='-1'"); else if (def->data.vnc.port) virBufferVSprintf(buf, " port='%d'", def->data.vnc.port);
What I think we need is this:
if (def->data.vnc.port && (!def->data.vnc.autoport || def->id != -1)) virBufferVSprintf(buf, " port='%d'", def->data.vnc.port); else if (def->data.vnc.autiport) virBufferAddLit(buf, " port='-1'");
So the explicit port is output in XML if autoport is not set, or if the domain is running.
Okay +1, push the patch :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
So the explicit port is output in XML if autoport is not set, or if the domain is running.
Okay +1, push the patch :-)
Yes, I agree, I just tested out the patch and it seems to fix the issue for me. I'm going to commit the patch now. Chris Lalancette
On Thu, Jul 31, 2008 at 02:24:25PM +0200, Chris Lalancette wrote:
Daniel Veillard wrote:
So the explicit port is output in XML if autoport is not set, or if the domain is running.
Okay +1, push the patch :-)
Yes, I agree, I just tested out the patch and it seems to fix the issue for me. I'm going to commit the patch now.
You committed the wrong patch it appears - the original one instead of my revised version :-( Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Jul 31, 2008 at 02:17:25PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 31, 2008 at 02:24:25PM +0200, Chris Lalancette wrote:
Daniel Veillard wrote:
So the explicit port is output in XML if autoport is not set, or if the domain is running.
Okay +1, push the patch :-)
Yes, I agree, I just tested out the patch and it seems to fix the issue for me. I'm going to commit the patch now.
You committed the wrong patch it appears - the original one instead of my revised version :-(
Seems fixed now, I'm seeing case VIR_DOMAIN_GRAPHICS_TYPE_VNC: if (def->data.vnc.port && (!def->data.vnc.autoport || vm->id != -1)) virBufferVSprintf(buf, " port='%d'", def->data.vnc.port); else if (def->data.vnc.autoport) virBufferAddLit(buf, " port='-1'"); virBufferVSprintf(buf, " autoport='%s'", def->data.vnc.autoport ? "yes" : "no"); in CVS head, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
Seems fixed now, I'm seeing
case VIR_DOMAIN_GRAPHICS_TYPE_VNC: if (def->data.vnc.port && (!def->data.vnc.autoport || vm->id != -1)) virBufferVSprintf(buf, " port='%d'", def->data.vnc.port); else if (def->data.vnc.autoport) virBufferAddLit(buf, " port='-1'");
virBufferVSprintf(buf, " autoport='%s'", def->data.vnc.autoport ? "yes" : "no");
Yeah, Dan B committed the fix to the fix, since I didn't get around to it yesterday. Thanks Dan B! Chris Lalancette
participants (3)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard