[Libvir] generator.py -- why?
Hi all. So, I found a bug in the python bindings that I'd really like to fix, but when I sat down to do so I quickly found myself mired in a swampy mess of code generation: generator.py. :-) Now, I feel compelled to ask -- why don't we just have a static libvirt.py file that is WYSIWYG? The generator.py program alone is longer than the file it generates, and having a static file would not only make the code easier to read, but would also make bug fixing much simpler. But, I'm sure there's got to be a good reason for it. :-) Here's a program that produces the bug I tried to address: import libvirt def get_domain(dom_name): conn = libvirt.openReadOnly(None) domain = conn.lookupByName(dom_name) return domain d = get_domain("mydomain") print d.info() This is a fairly typical "factory" pattern I could imagine people using. The problem is that the connection object falls out of scope after the get_domain routine ends, and is therefore garbage collected. This leaves the "d" domain object with no valid connection, resulting in a "libvir: Xen error : failed Xen syscall ioctl 3166208" error. The simple solution would be for the connection object to pass a reference to itself into each domain object it creates. The domain objects would then maintain the reference until they are destroyed. I valiantly tried to follow the table-based patterns used in generator.py to fix this problem, but I found myself inventing somewhat arbitrary rules just to force the code I wanted it to output. And hence, $SUBJECT. :-) Pete
On Mon, Aug 28, 2006 at 11:53:09PM -0400, pvetere@redhat.com wrote:
Hi all. So, I found a bug in the python bindings that I'd really like to fix, but when I sat down to do so I quickly found myself mired in a swampy mess of code generation: generator.py. :-) Now, I feel compelled to ask -- why don't we just have a static libvirt.py file that is WYSIWYG? The generator.py program alone is longer than the file it generates, and having a static file would not only make the code easier to read, but would also make bug fixing much simpler. But, I'm sure there's got to be a good reason for it. :-)
Historic, the same generator is used by libxml2 and libxslt at least.
Here's a program that produces the bug I tried to address:
import libvirt def get_domain(dom_name): conn = libvirt.openReadOnly(None) domain = conn.lookupByName(dom_name) return domain d = get_domain("mydomain") print d.info()
This is a fairly typical "factory" pattern I could imagine people using. The problem is that the connection object falls out of scope after the get_domain routine ends, and is therefore garbage collected. This leaves the "d" domain object with no valid connection, resulting in a "libvir: Xen error : failed Xen syscall ioctl 3166208" error. The simple solution would be for the connection object to pass a reference to itself into each domain object it creates. The domain objects would then maintain the reference until they are destroyed.
Hum, right, but really even at the C level you want to keep the connection around as long as you manipulate the domain.
I valiantly tried to follow the table-based patterns used in generator.py to fix this problem, but I found myself inventing somewhat arbitrary rules just to force the code I wanted it to output.
And hence, $SUBJECT. :-)
Okay, this may look surprizing, but well ... Now we can probably add a reference back from the domain to the connection, but still dropping any handle to the connection is kind of weird. 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/
Quoting Daniel Veillard <veillard@redhat.com>:
On Mon, Aug 28, 2006 at 11:53:09PM -0400, pvetere@redhat.com wrote:
Hi all. So, I found a bug in the python bindings that I'd really like to fix, but when I sat down to do so I quickly found myself mired in a swampy mess of code generation: generator.py. [snip]
Historic, the same generator is used by libxml2 and libxslt at least.
Ah, ok. So, it's just a re-use of already-existing code. That makes me feel better. :-) Thanks for the background info.
Hum, right, but really even at the C level you want to keep the connection around as long as you manipulate the domain.
It sounds like you are suggesting that it might be better to add a back-reference in the underlying C code instead instead of just the Python code. Did I understand you correctly?
Okay, this may look surprizing, but well ... Now we can probably add a reference back from the domain to the connection, but still dropping any handle to the connection is kind of weird.
Yeah, it's not the best use case in the world, but certainly one that is possible, and one which I feel libvirt should probably account for. Pete
On Tue, Aug 29, 2006 at 08:15:28AM -0400, pvetere@redhat.com wrote:
Quoting Daniel Veillard <veillard@redhat.com>:
On Mon, Aug 28, 2006 at 11:53:09PM -0400, pvetere@redhat.com wrote:
Hi all. So, I found a bug in the python bindings that I'd really like to fix, but when I sat down to do so I quickly found myself mired in a swampy mess of code generation: generator.py. [snip]
Historic, the same generator is used by libxml2 and libxslt at least.
Ah, ok. So, it's just a re-use of already-existing code. That makes me feel better. :-) Thanks for the background info.
:-)
Hum, right, but really even at the C level you want to keep the connection around as long as you manipulate the domain.
It sounds like you are suggesting that it might be better to add a back-reference in the underlying C code instead instead of just the Python code. Did I understand you correctly?
I would let python do that by making sure domain classes have a reference to the connection class, then Python will manage the count by itself. Could you bugzilla this, so I don't forget ? 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:
I would let python do that by making sure domain classes have a reference to the connection class, then Python will manage the count by itself.
Could you bugzilla this, so I don't forget ?
Sure, I'll open up a BZ. Would you ever be potentially open to a patch which removes the code generation for the python stuff? I really think libvirt could offer a much more robust python library if the code wasn't generated. Pete
On Tue, Aug 29, 2006 at 10:45:03AM -0400, Peter Vetere wrote:
Daniel Veillard wrote:
I would let python do that by making sure domain classes have a reference to the connection class, then Python will manage the count by itself.
Could you bugzilla this, so I don't forget ?
Sure, I'll open up a BZ.
Would you ever be potentially open to a patch which removes the code generation for the python stuff? I really think libvirt could offer a much more robust python library if the code wasn't generated.
<grin/> not hermetically closed but I'm afraid of maintainance costs. 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/
On Mon, Aug 28, 2006 at 11:53:09PM -0400, pvetere@redhat.com wrote:
Hi all. So, I found a bug in the python bindings that I'd really like to fix, but when I sat down to do so I quickly found myself mired in a swampy mess of code generation: generator.py. :-) Now, I feel compelled to ask -- why don't we just have a static libvirt.py file that is WYSIWYG? The generator.py program alone is longer than the file it generates, and having a static file would not only make the code easier to read, but would also make bug fixing much simpler. But, I'm sure there's got to be a good reason for it. :-)
Here's a program that produces the bug I tried to address:
import libvirt def get_domain(dom_name): conn = libvirt.openReadOnly(None) domain = conn.lookupByName(dom_name) return domain d = get_domain("mydomain") print d.info()
This is a fairly typical "factory" pattern I could imagine people using. The
This is a also a very bad pattern to use. Not only is opening a new connection a fairly heavyweight opertion - it has to connect to xenstore, xend, and fork fork the proxy server. Now if each time to your get_domain the domain object returned is associated with a different connection object. This bypasseses the caching of domain object instances which is done internal to libvirt, degrading performance still further. 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 -=|
Quoting "Daniel P. Berrange" <berrange@redhat.com>: [snip]
Here's a program that produces the bug I tried to address:
import libvirt def get_domain(dom_name): conn = libvirt.openReadOnly(None) domain = conn.lookupByName(dom_name) return domain d = get_domain("mydomain") print d.info()
[snip] This is a also a very bad pattern to use. Not only is opening a new connection a fairly heavyweight opertion - it has to connect to xenstore, xend, and fork fork the proxy server. Now if each time to your get_domain the domain object returned is associated with a different connection object. This bypasseses the caching of domain object instances which is done internal to libvirt, degrading performance still further.
I agree with you completely. However, there might be legitimate architectural reasons for doing things the above way; it's possible that the programmer didn't want that connection-creation code cluttering up the program-proper ;-). IMHO, a robust library shouldn't make assumptions about how its users will use it. If the user wants to do something potentially dangerous, the library should at least allow it, and definitely not bomb out. Just my 2c. :-) Pete
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Peter Vetere -
pvetere＠redhat.com