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

[Daniel Veillard <veillard redhat com>]

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/