Re: [Libvir] Patch to attach/detach virtual devices on a running domain

On Tue, Nov 14, 2006 at 03:34:05PM +0100, michel.ponceau(a)bull.net wrote: 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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/