[Libvir] Add port number to <graphics> tag
We recently added the '<graphics>' tag to the XML description for HVM machines. Any apps using this info though would have to hardcode the assumption that Xen's QEMU listens on Domain-ID + 5900. Hardcoding the port numbers in the first place is already a really questionable decision, so we should avoid that propagating out to application code. Thus, the attached patch changes: <graphics type="vnc"/> So that it now looks like <graphics type="vnc" port="5905"/> The hardcoded 'Domain-ID + 5900' craziness is now at least isolated in libvirt, so provided apps take the port number from the XML they will be immune from changes in port numbering scheme. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Mon, Aug 07, 2006 at 04:20:04PM +0100, Daniel P. Berrange wrote:
We recently added the '<graphics>' tag to the XML description for HVM machines. Any apps using this info though would have to hardcode the assumption that Xen's QEMU listens on Domain-ID + 5900. Hardcoding the port numbers in the first place is already a really questionable decision, so we should avoid that propagating out to application code.
Thus, the attached patch changes:
<graphics type="vnc"/>
So that it now looks like
<graphics type="vnc" port="5905"/>
The hardcoded 'Domain-ID + 5900' craziness is now at least isolated in libvirt, so provided apps take the port number from the XML they will be immune from changes in port numbering scheme.
Okay, I nearly did that patch a couple of weeks ago, and then started wondering if the port could not be extracted from some informations provided by xend, but failed to finc anything in the xm --long output or on the xenstore data, and didn't made the change. I really think xend should provide the information, but agreed that's a first step toward sanity at the application level, feel free to commit (unless someone knows how to extract the port from xend !) Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, 2006-08-07 at 11:34 -0400, Daniel Veillard wrote:
Okay, I nearly did that patch a couple of weeks ago, and then started wondering if the port could not be extracted from some informations provided by xend, but failed to finc anything in the xm --long output or on the xenstore data, and didn't made the change. I really think xend should provide the information, but agreed that's a first step toward sanity at the application level, feel free to commit (unless someone knows how to extract the port from xend !)
It should be in xenstore now[1]... I saw the patch float by the end of last week. But we'll probably still want to fall back to this for versions of xend that didn't do so. Jeremy [1] Looks like under domainpath/console/vnc-port, commit is http://xenbits.xensource.com/xen-unstable.hg?cs=155385a02d0b
On Mon, Aug 07, 2006 at 11:55:00AM -0400, Jeremy Katz wrote:
On Mon, 2006-08-07 at 11:34 -0400, Daniel Veillard wrote:
Okay, I nearly did that patch a couple of weeks ago, and then started wondering if the port could not be extracted from some informations provided by xend, but failed to finc anything in the xm --long output or on the xenstore data, and didn't made the change. I really think xend should provide the information, but agreed that's a first step toward sanity at the application level, feel free to commit (unless someone knows how to extract the port from xend !)
It should be in xenstore now[1]... I saw the patch float by the end of last week. But we'll probably still want to fall back to this for versions of xend that didn't do so.
Jeremy
[1] Looks like under domainpath/console/vnc-port, commit is http://xenbits.xensource.com/xen-unstable.hg?cs=155385a02d0b
Ha ha :-) that look way saner to me, except nobody but root can really get the information (IIRC by default the xenstore _ro socket is not accessible). Now should we still hardcode 5900 + id if not available ? The XML dump should go though the proxy too for non-root that's something I need to add too. Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, Aug 07, 2006 at 12:11:17PM -0400, Daniel Veillard wrote:
On Mon, Aug 07, 2006 at 11:55:00AM -0400, Jeremy Katz wrote:
On Mon, 2006-08-07 at 11:34 -0400, Daniel Veillard wrote:
Okay, I nearly did that patch a couple of weeks ago, and then started wondering if the port could not be extracted from some informations provided by xend, but failed to finc anything in the xm --long output or on the xenstore data, and didn't made the change. I really think xend should provide the information, but agreed that's a first step toward sanity at the application level, feel free to commit (unless someone knows how to extract the port from xend !)
It should be in xenstore now[1]... I saw the patch float by the end of last week. But we'll probably still want to fall back to this for versions of xend that didn't do so.
Jeremy
[1] Looks like under domainpath/console/vnc-port, commit is http://xenbits.xensource.com/xen-unstable.hg?cs=155385a02d0b
Ha ha :-)
that look way saner to me, except nobody but root can really get the information (IIRC by default the xenstore _ro socket is not accessible).
If the proxy has support for getting the XML it shouldn't matter ?
Now should we still hardcode 5900 + id if not available ?
Yeah, I think we need to be able to guarentee that whenever there is a '<graphics type=vnc>' tag, it will always have a port number attribute. If the port number is missing some of the time, people won't be inlined to make use of it & will go back to hardcoding stuff in the application code instead.
The XML dump should go though the proxy too for non-root that's something I need to add too.
Yeah, I just noticed that that's going to be needed too. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Mon, Aug 07, 2006 at 12:11:17PM -0400, Daniel Veillard wrote:
On Mon, Aug 07, 2006 at 11:55:00AM -0400, Jeremy Katz wrote:
On Mon, 2006-08-07 at 11:34 -0400, Daniel Veillard wrote:
Okay, I nearly did that patch a couple of weeks ago, and then started wondering if the port could not be extracted from some informations provided by xend, but failed to finc anything in the xm --long output or on the xenstore data, and didn't made the change. I really think xend should provide the information, but agreed that's a first step toward sanity at the application level, feel free to commit (unless someone knows how to extract the port from xend !)
It should be in xenstore now[1]... I saw the patch float by the end of last week. But we'll probably still want to fall back to this for versions of xend that didn't do so.
Jeremy
[1] Looks like under domainpath/console/vnc-port, commit is http://xenbits.xensource.com/xen-unstable.hg?cs=155385a02d0b
Ha ha :-)
that look way saner to me, except nobody but root can really get the information (IIRC by default the xenstore _ro socket is not accessible).
This actually raises an interesting question. The current code for constructing the XML for a domain is done in the XenD backend - xend_internal.c Extracting the port, however, would require talking to XenStore. All the XenStore related code, however, is in a different driver backend xs_internal.c So how should we go about implementing this ? Can we have code in xend_internal.c that talks to XenStore to extract the VNC port ? Or should we have XML generation code in xs_internal.c too & somehow merge the 2 XML docs (yuk). This is the same problem we so far prevented me adding the path to the serial console Psuedo-TTY to the XML for a domain. I'd rather like to have this in 0.1.4 release of libvirt too, so any suggestions on how to approach implementation when some of the data for the XML needs to come from XS ? Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Mon, Aug 07, 2006 at 10:12:02PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 07, 2006 at 12:11:17PM -0400, Daniel Veillard wrote:
On Mon, Aug 07, 2006 at 11:55:00AM -0400, Jeremy Katz wrote:
It should be in xenstore now[1]... I saw the patch float by the end of last week. But we'll probably still want to fall back to this for versions of xend that didn't do so.
Jeremy
[1] Looks like under domainpath/console/vnc-port, commit is http://xenbits.xensource.com/xen-unstable.hg?cs=155385a02d0b
Ha ha :-)
that look way saner to me, except nobody but root can really get the information (IIRC by default the xenstore _ro socket is not accessible).
This actually raises an interesting question. The current code for constructing the XML for a domain is done in the XenD backend - xend_internal.c Extracting the port, however, would require talking to XenStore. All the XenStore related code, however, is in a different driver backend xs_internal.c
Annoying isn't it ;-) ?
So how should we go about implementing this ? Can we have code in xend_internal.c that talks to XenStore to extract the VNC port ?
yes, one must check first that the xenstore connection is open (or the read-only one in the proxy this is a TODO it wasn't needed until now) and do the cross call. I think there is little to gain there trying to reach absolute purity within the
Or should we have XML generation code in xs_internal.c too & somehow merge the 2 XML docs (yuk).
In 9 years of libxml I managed to resist trying to do the merge operation, I am not gonna try now :-)
This is the same problem we so far prevented me adding the path to the serial console Psuedo-TTY to the XML for a domain. I'd rather like to have this in 0.1.4 release of libvirt too, so any suggestions on how to approach implementation when some of the data for the XML needs to come from XS ?
I would make a wrapper function for the 2 special xs_read calls, to only export relatively high level interface from xs_internal.h for example int xenStoreDomainVncPort(virDomainPtr domain); returning -1 in case of error. Similar for the TTY. Make sense ? Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, Aug 07, 2006 at 05:35:18PM -0400, Daniel Veillard wrote:
On Mon, Aug 07, 2006 at 10:12:02PM +0100, Daniel P. Berrange wrote:
So how should we go about implementing this ? Can we have code in xend_internal.c that talks to XenStore to extract the VNC port ?
yes, one must check first that the xenstore connection is open (or the read-only one in the proxy this is a TODO it wasn't needed until now) and do the cross call. I think there is little to gain there trying to reach absolute purity within the
Yeah, that was pretty much what I was expecting.
This is the same problem we so far prevented me adding the path to the serial console Psuedo-TTY to the XML for a domain. I'd rather like to have this in 0.1.4 release of libvirt too, so any suggestions on how to approach implementation when some of the data for the XML needs to come from XS ?
I would make a wrapper function for the 2 special xs_read calls, to only export relatively high level interface from xs_internal.h for example
int xenStoreDomainVncPort(virDomainPtr domain);
returning -1 in case of error. Similar for the TTY.
Make sense ?
Yeah sounds resonable to me. I'll take a crack at implementing these two additions to the XML doc and post a patch for review when I have something reasonable working. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Mon, Aug 07, 2006 at 10:40:11PM +0100, Daniel P. Berrange wrote:
This is the same problem we so far prevented me adding the path to the serial console Psuedo-TTY to the XML for a domain. I'd rather like to have this in 0.1.4 release of libvirt too, so any suggestions on how to approach implementation when some of the data for the XML needs to come from XS ?
I would make a wrapper function for the 2 special xs_read calls, to only export relatively high level interface from xs_internal.h for example
int xenStoreDomainVncPort(virDomainPtr domain);
returning -1 in case of error. Similar for the TTY.
Make sense ?
Yeah sounds resonable to me. I'll take a crack at implementing these two additions to the XML doc and post a patch for review when I have something reasonable working.
See attached patch which looks up port number in xenstore & also pulls out the serial console tty. The example XML from a HVM domain now looks like: <domain type='xen' id='7'> <name>twit</name> <uuid>12cf7ed6915818803eaa98ad00bd3fc5</uuid> <os> <type>hvm</type> <loader>/usr/lib/xen/boot/hvmloader</loader> <boot dev='/dev/cdrom'/> </os> <memory>512000</memory> <vcpu>1</vcpu> <on_poweroff>destroy</on_poweroff> <on_reboot>restart</on_reboot> <on_crash>restart</on_crash> <devices> <emulator>/usr/lib64/xen/bin/qemu-dm</emulator> <interface type='bridge'> <source bridge='xenbr0'/> <mac address='00:16:3e:52:b2:82'/> <script path='vif-bridge'/> </interface> <disk type='file'> <source file='/root/rhel3.img'/> <target dev='ioemu:hda'/> </disk> <graphics type='vnc' port='5934'/> <console tty='/dev/pts/7'/> </devices> </domain> The '<console>' element is actually available for both HVM and PV domains. I also updated format.html with details of these XML elements / attributes. NB, the version of xenstore I'm running doesn't actually include the changeset 155385a02d0b which writes the vnc-port. Thus to test it I manually wrote an entry in the same location. xenstore-write /local/domain/7/console/vnc-port 5934 Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Mon, Aug 07, 2006 at 11:41:48PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 07, 2006 at 10:40:11PM +0100, Daniel P. Berrange wrote:
This is the same problem we so far prevented me adding the path to the serial console Psuedo-TTY to the XML for a domain. I'd rather like to have this in 0.1.4 release of libvirt too, so any suggestions on how to approach implementation when some of the data for the XML needs to come from XS ?
I would make a wrapper function for the 2 special xs_read calls, to only export relatively high level interface from xs_internal.h for example
int xenStoreDomainVncPort(virDomainPtr domain);
returning -1 in case of error. Similar for the TTY.
Make sense ?
Yeah sounds resonable to me. I'll take a crack at implementing these two additions to the XML doc and post a patch for review when I have something reasonable working.
See attached patch which looks up port number in xenstore & also pulls out the serial console tty.
looks perfect, I would just nitpick about the use of atoi which doesn't catch errors (strtol is better I guess). And also the fact that the patch to format.html should really be applied to libvir.html because all htmls are generated from it via the stylesheets (make rebuild in doc directory). thanks ! Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Tue, Aug 08, 2006 at 10:31:38AM -0400, Daniel Veillard wrote:
On Mon, Aug 07, 2006 at 11:41:48PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 07, 2006 at 10:40:11PM +0100, Daniel P. Berrange wrote:
Yeah sounds resonable to me. I'll take a crack at implementing these two additions to the XML doc and post a patch for review when I have something reasonable working.
See attached patch which looks up port number in xenstore & also pulls out the serial console tty.
looks perfect, I would just nitpick about the use of atoi which doesn't catch errors (strtol is better I guess). And also the fact that the patch to format.html should really be applied to libvir.html because all htmls are generated from it via the stylesheets (make rebuild in doc directory).
Attached an updated patch using strol (i had copied the use of atoi from other methods in that xs_internal.h file though ;-). Also made the changes to libvir.html instead. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Tue, Aug 08, 2006 at 04:15:40PM +0100, Daniel P. Berrange wrote:
On Tue, Aug 08, 2006 at 10:31:38AM -0400, Daniel Veillard wrote:
looks perfect, I would just nitpick about the use of atoi which doesn't catch errors (strtol is better I guess). And also the fact that the patch to format.html should really be applied to libvir.html because all htmls are generated from it via the stylesheets (make rebuild in doc directory).
Attached an updated patch using strol (i had copied the use of atoi from other methods in that xs_internal.h file though ;-). Also made the changes
I told you I was nitpicking :-)
to libvir.html instead.
Looks all fine to me ! Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, Aug 07, 2006 at 12:11:17PM -0400, Daniel Veillard wrote:
The XML dump should go though the proxy too for non-root that's something
Attached is a patch which adds support for XML dump to the proxy. This code only works if XML doc is < 4k, but I don;t anticpate this being a problem. While doing this I also added a tonne more #ifndef PROXY statements around functions in xend_internal.c since there were a lot of potentially dangerous functions being compiled into the proxy even though they were not called. I've run 'nm' on the .o files linked by the proxy and it looks to have cut down the list of functions quite significantly. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Tue, Aug 08, 2006 at 11:04:44PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 07, 2006 at 12:11:17PM -0400, Daniel Veillard wrote:
The XML dump should go though the proxy too for non-root that's something
Attached is a patch which adds support for XML dump to the proxy. This code only works if XML doc is < 4k, but I don;t anticpate this being a problem.
yeah that was the only think I was afraid could be a problem, but in practice the size of a doman description really should not be over 4k.
While doing this I also added a tonne more #ifndef PROXY statements around functions in xend_internal.c since there were a lot of potentially dangerous functions being compiled into the proxy even though they were not called. I've run 'nm' on the .o files linked by the proxy and it looks to have cut down the list of functions quite significantly.
Very good idea ! Patch looks good to me, but:
Index: src/driver.h =================================================================== RCS file: /data/cvs/libvirt/src/driver.h,v retrieving revision 1.10 diff -c -r1.10 driver.h *** src/driver.h 8 Aug 2006 22:22:55 -0000 1.10 --- src/driver.h 8 Aug 2006 22:53:55 -0000 *************** *** 104,109 **** --- 104,112 ---- typedef int (*virDrvDomainRestore) (virConnectPtr conn, const char *from); + typedef char * + (*virDrvDomainDumpXML) (virDomainPtr dom, + int flags);
typedef int (*virDrvDomainSetVcpus) (virDomainPtr domain, *************** *** 164,169 **** --- 167,173 ---- virDrvDomainSetVcpus domainSetVcpus; virDrvDomainPinVcpu domainPinVcpu; virDrvDomainGetVcpus domainGetVcpus; + virDrvDomainDumpXML domainDumpXML; };
that and the updates of the driver tables will conflict with the change I just commited to migrate vCPU and affinity functions to the driver system too. Sorry :-)
! NULL, /* domainGetVcpus */ ! xenProxyDomainDumpXML, /* domainDumpXML */ };
usually I drop the coma after the last field of a structure. I force gcc in a very pedantic mode and it complains about this :-)
! VIR_PROXY_DOMAIN_INFO = 9, ! VIR_PROXY_DOMAIN_XML = 10, } virProxyCommand;
same. and in the driver tables too. Don't worry about it I will fix the warning when I see them :-) Your patch also includes other changes already commited about TTY and VNC port extraction, maybe you were not working from a CVS fully updated ? but that looks good, the merge may be a bit annoying though :-) Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Tue, Aug 08, 2006 at 06:19:16PM -0400, Daniel Veillard wrote:
On Tue, Aug 08, 2006 at 11:04:44PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 07, 2006 at 12:11:17PM -0400, Daniel Veillard wrote: diff -c -r1.10 driver.h *** src/driver.h 8 Aug 2006 22:22:55 -0000 1.10 --- src/driver.h 8 Aug 2006 22:53:55 -0000 *************** *** 104,109 **** --- 104,112 ---- typedef int (*virDrvDomainRestore) (virConnectPtr conn, const char *from); + typedef char * + (*virDrvDomainDumpXML) (virDomainPtr dom, + int flags);
typedef int (*virDrvDomainSetVcpus) (virDomainPtr domain, *************** *** 164,169 **** --- 167,173 ---- virDrvDomainSetVcpus domainSetVcpus; virDrvDomainPinVcpu domainPinVcpu; virDrvDomainGetVcpus domainGetVcpus; + virDrvDomainDumpXML domainDumpXML; };
that and the updates of the driver tables will conflict with the change I just commited to migrate vCPU and affinity functions to the driver system too. Sorry :-)
Actually it should't - I updated to CVS HEAD and merged wrt to your changes just before submitting this patch, so I think its OK.
! NULL, /* domainGetVcpus */ ! xenProxyDomainDumpXML, /* domainDumpXML */ };
usually I drop the coma after the last field of a structure. I force gcc in a very pedantic mode and it complains about this :-)
I prefer to keep the trailing comma because it means one-line less diff next time we add more ffunctions to the driver API. I'll chop them out though to match your existing coding style in libvirt sources.
Your patch also includes other changes already commited about TTY and VNC port extraction, maybe you were not working from a CVS fully updated ?
but that looks good, the merge may be a bit annoying though :-)
I needed to modify some of the signatures on the methods I added for TTY and VNC port extraction because the proxy daemon doesn't have access to the full virDomainPtr object, only the virConnectPtr and domain ID. So that's why these functiosn appear in the diff - the patch was taken from 'cvs diff -c' against a checkout updated to HEAD so I don;t expect any merge issues. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Wed, Aug 09, 2006 at 01:13:25AM +0100, Daniel P. Berrange wrote:
On Tue, Aug 08, 2006 at 06:19:16PM -0400, Daniel Veillard wrote:
that and the updates of the driver tables will conflict with the change I just commited to migrate vCPU and affinity functions to the driver system too. Sorry :-)
Actually it should't - I updated to CVS HEAD and merged wrt to your changes just before submitting this patch, so I think its OK.
Okay, maybe I just got confused :-) Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jeremy Katz