8:34 a.m.
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(a)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.
9:05 a.m.
On Tue, Nov 14, 2006 at 03:34:05PM +0100, michel.ponceau(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
11:19 a.m.
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(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
9:50 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
12:36 p.m.
On Tue, Nov 14, 2006 at 03:34:05PM +0100, michel.ponceau(a)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 -=|
6390
days inactive
6396
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
michel.ponceau＠bull.net