[Libvir] Patch to attach/detach virtual devices on a running domain
This patch adds to Libvirt-0.1.8 the two functions described in my preceding mail. It has been tested in our Bull environment. Michel Ponceau 02/10/2006 17:27 Pour : libvir-list@redhat.com cc : Objet : Proposal for virtual devices described in XML Proposal modified after comments from Daniel P. Berrange: /* Create a virtual device attachment to backend */ int virDomainAttachDevice(virDomainPtr domain, char *xml); /* Destroy a virtual device attachment to backend */ int virDomainDetachDevice(virDomainPtr domain, char *xml); /* @domain: pointer to domain object * @xml: pointer to XML description of one device * Returns 0 in case of success, -1 in case of failure. */ The XML is in same form as the part of domain description for a single device, either <disk.../disk> or <interface.../interface>. In Detach function, the device is identified by its target name for "disk", and its MAC address for "interface". When the MAC address has not been assigned by Libvirt user at device creation, it must be retrieved from Xen using virDomainGetXMLDesc function.
On Tue, Nov 14, 2006 at 03:34:05PM +0100, michel.ponceau@bull.net wrote:
This patch adds to Libvirt-0.1.8 the two functions described in my preceding mail. It has been tested in our Bull environment.
There have been no negative comments to the interfaces proposed, so I guess we can assume that is okay, I'm still a bit vary of using XML for the Detach operation, but this avoid having to add the concept of device object at the API level right now, and keep things a bit simpler. Okay, here is a few remarks on the content of the patch: 1/ DOS end of lines in the patch, in general it's safer to do the diffs and code changes on a Unix box to avoid troubles :-) 2/ The public functions call directly the xenDaemon... implementation routines, that's not proper, everything need to go though the driver indirection, i.e. the driver structures need to be extended and the new functions need to be registered that way 3/ // type of code comments are banned in my code, sorry :-) 4/ virDomainXMLDevID() seems a bit weak, for example it seems to work only on disk registered though a physical device, though I think there is a need for (un)registering file based domU devices. For vif I wonder if the test on the mac address is really the only way to select the proper device, that's probably sufficient. 5/ virDomainXMLDevID does direct acces to xs_directory() and xs_read(), that's not proper it should not call xenStore directly that need to be cleaned up, define one high level function exported from xs_internal.[ch] and call that but no direct access should be done that leads to unmaintainable code. and a couple of tiny things left and right, which I can take care of. Basically 1/ 2/ 3/ are not blockers, I can fix those, for 4/ the limitations need at least to be documented, better fixed. 5/ really need to be fixed. I'm not against starting to work now to integrate that patch but I need 5 (and preferably 4/) to be fixed, if you agree I will work on the other points in parallel, 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 Tue, Nov 14, 2006 at 10:05:47AM -0500, Daniel Veillard wrote:
On Tue, Nov 14, 2006 at 03:34:05PM +0100, michel.ponceau@bull.net wrote:
This patch adds to Libvirt-0.1.8 the two functions described in my preceding mail. It has been tested in our Bull environment.
There have been no negative comments to the interfaces proposed, so I guess we can assume that is okay, I'm still a bit vary of using XML for the Detach operation, but this avoid having to add the concept of device object at the API level right now, and keep things a bit simpler.
Okay, here is a few remarks on the content of the patch:
1/ DOS end of lines in the patch, in general it's safer to do the diffs and code changes on a Unix box to avoid troubles :-) 2/ The public functions call directly the xenDaemon... implementation routines, that's not proper, everything need to go though the driver indirection, i.e. the driver structures need to be extended and the new functions need to be registered that way 3/ // type of code comments are banned in my code, sorry :-)
Okay, I applied the patches selectively and did the changes 1/ to 3/, as a result it changes more files since the driver structure grew.
5/ virDomainXMLDevID does direct acces to xs_directory() and xs_read(), that's not proper it should not call xenStore directly that need to be cleaned up, define one high level function exported from xs_internal.[ch] and call that but no direct access should be done that leads to unmaintainable code.
That still need to be isolated, I added a TODO in src/xml.c to this effect. thanks ! 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 Thu, Nov 16, 2006 at 12:19:13PM -0500, Daniel Veillard wrote:
On Tue, Nov 14, 2006 at 10:05:47AM -0500, Daniel Veillard wrote:
5/ virDomainXMLDevID does direct acces to xs_directory() and xs_read(), that's not proper it should not call xenStore directly that need to be cleaned up, define one high level function exported from xs_internal.[ch] and call that but no direct access should be done that leads to unmaintainable code.
That still need to be isolated, I added a TODO in src/xml.c to this effect.
Okay I have done it, as a new function xenStoreDomainGetNetworkID() from xs_internal.[ch] Could you check out the version from CVS and verify I didn't added any regression ? thanks, 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 Tue, Nov 14, 2006 at 03:34:05PM +0100, michel.ponceau@bull.net wrote:
This patch adds to Libvirt-0.1.8 the two functions described in my preceding mail. It has been tested in our Bull environment.
This looks good to me - only one small bug I see - in the virParseXMLDevice function there is a memory leak. If you take 'goto errror' path through the function, then the xmlDocPtr object will never be free'd. Thanks for taking the time to implement all this ! 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 -=|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
michel.ponceau＠bull.net