[libvirt] anyone implementing host device enumeration?
Hi - I'm about to start working on host device enumeration, along the (HAL-ish) lines of what was discussed back in April: https://www.redhat.com/archives/libvir-list/2008-April/msg00005.html I know the xml details haven't been fully fleshed out, but there seems to be agreement that it will be a fairly direct mapping from (a subset of the) HAL info to the details that we need in the xml. Doubtless it will take a while to figure out exactly what subset suffices (and, for that matter, if everything needed is available via HAL ...), but I think the work is well-defined for some of the obvious details (discussed in the above thread) on which there's broad agreement. Is anyone working on such an implementation? Thanks, Dave
On Thu, Aug 21, 2008 at 03:18:57PM -0400, David Lively wrote:
Hi -
I'm about to start working on host device enumeration, along the (HAL-ish) lines of what was discussed back in April: https://www.redhat.com/archives/libvir-list/2008-April/msg00005.html
I know the xml details haven't been fully fleshed out, but there seems to be agreement that it will be a fairly direct mapping from (a subset of the) HAL info to the details that we need in the xml. Doubtless it will take a while to figure out exactly what subset suffices (and, for that matter, if everything needed is available via HAL ...), but I think the work is well-defined for some of the obvious details (discussed in the above thread) on which there's broad agreement.
Is anyone working on such an implementation?
No one that I'm aware of - so go for it. FYI, one complication has come up since my initial proposal. HAL is going away, to be replaced by DeviceKit :-( That said they will co-exist and HAL will be around for a while and they will both expose very similar properties for devices so it shouldn't too much trouble. I think we just need to be conservative when deciding how many of the HAL properties we need to add in libvirt. I'd go for a pretty minimal set at first. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Aug 22, 2008 at 09:44:47AM +0100, Daniel P. Berrange wrote:
On Thu, Aug 21, 2008 at 03:18:57PM -0400, David Lively wrote:
Hi -
I'm about to start working on host device enumeration, along the (HAL-ish) lines of what was discussed back in April: https://www.redhat.com/archives/libvir-list/2008-April/msg00005.html
I know the xml details haven't been fully fleshed out, but there seems to be agreement that it will be a fairly direct mapping from (a subset of the) HAL info to the details that we need in the xml. Doubtless it will take a while to figure out exactly what subset suffices (and, for that matter, if everything needed is available via HAL ...), but I think the work is well-defined for some of the obvious details (discussed in the above thread) on which there's broad agreement.
Is anyone working on such an implementation?
No one that I'm aware of - so go for it. FYI, one complication has come up since my initial proposal. HAL is going away, to be replaced by DeviceKit :-( That said they will co-exist and HAL will be around for a while and they will both expose very similar properties for devices so it shouldn't too much trouble. I think we just need to be conservative when deciding how many of the HAL properties we need to add in libvirt. I'd go for a pretty minimal set at first.
And HAL based detection might work on more platforms at least initially like OpenSolaris, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Aug 21, 2008 at 03:18:57PM -0400, David Lively wrote:
Hi -
I'm about to start working on host device enumeration, along the (HAL-ish) lines of what was discussed back in April: https://www.redhat.com/archives/libvir-list/2008-April/msg00005.html
I know the xml details haven't been fully fleshed out, but there seems to be agreement that it will be a fairly direct mapping from (a subset of the) HAL info to the details that we need in the xml. Doubtless it will take a while to figure out exactly what subset suffices (and, for that matter, if everything needed is available via HAL ...), but I think the work is well-defined for some of the obvious details (discussed in the above thread) on which there's broad agreement.
Is anyone working on such an implementation?
Did you ever start any work on this project ? The oVirt guys really want this done asap, so if you've not started on it, or have a partial start to work from, I plan to make time to look at it next week Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Hi Daniel - I have an implementation underway right now, with both HAL- and Devkit-based "drivers". We have some folks who need this *next week*, so I'm trying to get the most functionality finished up today and this weekend. The generic (HAL/Devkit-agnostic) framework is essentially done and tested (except for a few odds and ends like some missing virsh fns). The HAL-based driver is ~80% done. The Devkit-based driver isn't very capable yet, mostly because of the limited Devkit functionality available (also, there are some serious naming issues to discuss). I haven't yet implemented virNodeDevice{Create,Destroy}, nor am I registering for HAL/Devkit events. We don't need this functionality right away, so I may wait 'til next week to implement them. The underlying virNodeDevice infrastructure is ref-counted and accessed via a conn->lock-protected virHashTable (like connections, domains, etc.), so concurrent access shouldn't be a problem. I'll post a patch (with some TODOs ...) on Sunday or Monday morning. Dave On Fri, 2008-09-26 at 14:15 +0100, Daniel P. Berrange wrote:
On Thu, Aug 21, 2008 at 03:18:57PM -0400, David Lively wrote:
Hi -
I'm about to start working on host device enumeration, along the (HAL-ish) lines of what was discussed back in April: https://www.redhat.com/archives/libvir-list/2008-April/msg00005.html
I know the xml details haven't been fully fleshed out, but there seems to be agreement that it will be a fairly direct mapping from (a subset of the) HAL info to the details that we need in the xml. Doubtless it will take a while to figure out exactly what subset suffices (and, for that matter, if everything needed is available via HAL ...), but I think the work is well-defined for some of the obvious details (discussed in the above thread) on which there's broad agreement.
Is anyone working on such an implementation?
Did you ever start any work on this project ? The oVirt guys really want this done asap, so if you've not started on it, or have a partial start to work from, I plan to make time to look at it next week
Regards, Daniel
Hi Daniel - I'm not really ready to submit this yet (hence no [PATCH] in the subject), but it is functional enough to play with, mostly with the HAL-based driver, though the devkit-based one works too. In particular, I'm not yet happy with the code to gather capabilities and bus info (though gather_capabilities in node_device_hal.c is close to what I'm going for). Also, many of the details (which properties do we care about for which buses and capabilities?) that make this useful are missing, though I've provided a few guesses to get started. And finally, it's had very little testing at this point. Configure --with-hal or --with-devkit to support one or both (if both are configured, HAL is preferred, provided we can talk to hald on dbus). There are quite a few TODO's in the patch (mostly little things). The larger TODO's are (in (my) priority order): * generalize gather_capabilities (node_device_hal.c) to work for HAL bus info as well * share the nodeDevices hash table among (non-remote) connections (perhaps store it in the virNodeDriver?) * register for HAL events to keep device info up-to-date * finish virsh, python support (for now there's enough to get devs/xml) * support more capability & bus properties (in HAL) * generalize gather_capabilities to work for gathering bus & capability info for both HAL & devkit drivers * figure out how devkit and HAL correlate, so we report device info consistently * register for devkit events to keep device info up-to-date Dave P.S. I'm afraid the patch is rather large, but remember to skip the generated files when looking it over: configure.in | 97 ++++++ include/libvirt/libvirt.h | 78 +++++ include/libvirt/libvirt.h.in | 78 +++++ include/libvirt/virterror.h | 4 python/generator.py | 15 + python/libvir.c | 127 ++++++++ python/libvirt-python-api.xml | 17 + python/libvirt_wrap.h | 10 python/types.c | 15 + qemud/remote.c | 282 +++++++++++++++++++ qemud/remote_dispatch_localvars.h | 20 + qemud/remote_dispatch_proc_switch.h | 93 ++++++ qemud/remote_dispatch_prototypes.h | 11 qemud/remote_protocol.c | 220 +++++++++++++++ qemud/remote_protocol.h | 184 ++++++++++++ qemud/remote_protocol.x | 117 +++++++ src/Makefile.am | 19 + src/driver.h | 67 ++++ src/hash.c | 181 ++++++++++++ src/internal.h | 52 +++ src/libvirt.c | 528 +++++++++++++++++++++++++++++++++++\ - src/libvirt_sym.version | 14 src/node_device.c | 262 +++++++++++++++++ src/node_device.h | 38 ++ src/node_device_devkit.c | 299 ++++++++++++++++++++ src/node_device_hal.c | 475 ++++++++++++++++++++++++++++++++ src/remote_internal.c | 364 ++++++++++++++++++++++++ src/virsh.c | 80 +++++ src/virterror.c | 21 + 29 files changed, 3757 insertions(+), 11 deletions(-) On Fri, 2008-09-26 at 14:15 +0100, Daniel P. Berrange wrote:
On Thu, Aug 21, 2008 at 03:18:57PM -0400, David Lively wrote:
Hi -
I'm about to start working on host device enumeration, along the (HAL-ish) lines of what was discussed back in April: https://www.redhat.com/archives/libvir-list/2008-April/msg00005.html
I know the xml details haven't been fully fleshed out, but there seems to be agreement that it will be a fairly direct mapping from (a subset of the) HAL info to the details that we need in the xml. Doubtless it will take a while to figure out exactly what subset suffices (and, for that matter, if everything needed is available via HAL ...), but I think the work is well-defined for some of the obvious details (discussed in the above thread) on which there's broad agreement.
Is anyone working on such an implementation?
Did you ever start any work on this project ? The oVirt guys really want this done asap, so if you've not started on it, or have a partial start to work from, I plan to make time to look at it next week
Regards, Daniel
On Tue, Sep 30, 2008 at 12:17:27AM -0400, David Lively wrote:
Hi Daniel -
I'm not really ready to submit this yet (hence no [PATCH] in the subject), but it is functional enough to play with, mostly with the HAL-based driver, though the devkit-based one works too. In particular, I'm not yet happy with the code to gather capabilities and bus info (though gather_capabilities in node_device_hal.c is close to what I'm going for). Also, many of the details (which properties do we care about for which buses and capabilities?) that make this useful are missing, though I've provided a few guesses to get started. And finally, it's had very little testing at this point.
Okay, I think giving a bit of feedback at least on the API entry points makes sense even if not complete.
Configure --with-hal or --with-devkit to support one or both (if both are configured, HAL is preferred, provided we can talk to hald on dbus). There are quite a few TODO's in the patch (mostly little things).
The set of dependancies for libvirt is becoming incredibly large but that's fine :-)
* figure out how devkit and HAL correlate, so we report device info consistently
that has the potential of being nasty like seeing devices listed twice
* register for devkit events to keep device info up-to-date
okay it looks like there is still some work, but thanks a lot for sharing that first version, very appreciated ! Good to see that you seems to have the python bindings ready too !
+/* + * Host device enumeration + */ + +/** + * virNodeDevice: + * + * A virNodeDevice contains a node (host) device details. + */ + +typedef struct _virNodeDevice virNodeDevice; + +/** + * virNodeDevicePtr: + * + * A virNodeDevicePtr is a pointer to a virNodeDevice structure. Get + * one via virNodeDeviceLookupByKey, virNodeDeviceLookupByName, or + * virNodeDeviceCreate. Be sure to Call virNodeDeviceFree when done + * using a virNodeDevicePtr obtained from any of the above functions to + * avoid leaking memory. + */ [...] +const char * virNodeDeviceGetKey (virNodeDevicePtr dev); + +const char * virNodeDeviceGetName (virNodeDevicePtr dev); + +const char * virNodeDeviceGetParentKey(virNodeDevicePtr dev); + +const char * virNodeDeviceGetBus (virNodeDevicePtr dev); + +int virNodeDeviceNumOfCaps (virNodeDevicePtr dev); + +int virNodeDeviceListCaps (virNodeDevicePtr dev, + char **const names, + int maxnames); + +char * virNodeDeviceGetXMLDesc (virNodeDevicePtr dev, + unsigned int flags); +
Opaque type with accessors, looks fine to me !
+typedef virNodeDevice *virNodeDevicePtr; + + +int virNodeNumOfDevices (virConnectPtr conn); + +int virNodeListDevices (virConnectPtr conn, + char **const names, + int maxnames); + +int virNodeNumOfDevicesByCap (virConnectPtr conn, + const char *cap); + +int virNodeListDevicesByCap (virConnectPtr conn, + const char *cap, + char **const names, + int maxnames); + +int virNodeNumOfDevicesByBus (virConnectPtr conn, + const char *bus); + +int virNodeListDevicesByBus (virConnectPtr conn, + const char *bus, + char **const names, + int maxnames); + +virNodeDevicePtr virNodeDeviceLookupByName (virConnectPtr conn, + const char *name); + +virNodeDevicePtr virNodeDeviceLookupByKey (virConnectPtr conn, + const char *key); + +virNodeDevicePtr virNodeDeviceCreate (virConnectPtr conn, + const char *xml);
I wonder how many of them should be future-proofed by adding them a final flags argument too ... For example it may be useful to only lookup devices which are local to the machine, or the opposite only remote devices. We don't have to specify now flags values, but having the APIs ready is sufficient. The virNodeNum/virNodeListDevices devices can probably all share the same flags definitions when needed. The LookupByName/LookupByKey may be able to use the same set. I wonder a bit about the key argument though, I assume it's something actually defined by the lower APIs (hal/devkit). For virNodeDeviceCreate maybe a flags may be needed too, though the flexibility of the API is provided by the XML.
+int virNodeDeviceDestroy (virNodeDevicePtr dev); + +int virNodeDeviceFree (virNodeDevicePtr dev);
Maybe Destroy need flags too, for example to force (or not) destruction of devices which may be in use.
@@ -332,6 +340,12 @@ skip_function = ( 'virCopyLastError', # Python API is called virGetLastError instead 'virConnectOpenAuth', # Python C code is manually written 'virDefaultErrorFunc', # Python virErrorFuncHandler impl calls this from C + 'virNodeDeviceGetKey', + 'virNodeDeviceGetName', + 'virNodeDeviceGetParentKey', + 'virNodeDeviceGetBus', + 'virNodeDeviceNumOfCaps', + 'virNodeDeviceListCaps', )
Strange how are the accessors supposed to work from python then ?
diff --git a/src/Makefile.am b/src/Makefile.am index 9845332..10fdd7d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,5 +1,18 @@ ## Process this file with automake to produce Makefile.in
+NODE_DEVICE_SOURCES = node_device.c node_device.h +if HAVE_HAL +NODE_DEVICE_CFLAGS = $(HAL_CFLAGS) +NODE_DEVICE_LIBS = $(HAL_LIBS) +NODE_DEVICE_SOURCES += node_device_hal.c +else +if HAVE_DEVKIT +NODE_DEVICE_CFLAGS = $(DEVKIT_CFLAGS) +NODE_DEVICE_LIBS = $(DEVKIT_LIBS) +NODE_DEVICE_SOURCES += node_device_devkit.c +endif +endif
Splitting into modules looks nice, [...]
$(GENERIC_LIB_SOURCES) \ $(DOMAIN_CONF_SOURCES) libvirt_lxc_LDFLAGS = $(WARN_CFLAGS) $(COVERAGE_LDCFLAGS) -libvirt_lxc_LDADD = $(LIBXML_LIBS) ../gnulib/lib/libgnu.la +libvirt_lxc_LDADD = $(LIBXML_LIBS) ../gnulib/lib/libgnu.la $(NODE_DEVICE_LIBS) libvirt_lxc_CFLAGS = $(LIBPARTED_CFLAGS)
Hum, the libvirt_lxc really need to link to the device discovery libs ?
index 655cd05..6a2ef82 100644 --- a/src/driver.h +++ b/src/driver.h @@ -601,6 +601,72 @@ struct _virStateDriver { }; #endif
+ +typedef struct _virNodeDriver virNodeDriver; +typedef virNodeDriver *virNodeDriverPtr; + +typedef int (*virDrvNodeNumOfDevices)(virConnectPtr conn); + +typedef int (*virDrvNodeListDevices)(virConnectPtr conn, + char **const names, + int maxnames); + +typedef int (*virDrvNodeNumOfDevicesByCap)(virConnectPtr conn, + const char *cap); + +typedef int (*virDrvNodeListDevicesByCap)(virConnectPtr conn, + const char *cap, + char **const names, + int maxnames); + +typedef int (*virDrvNodeNumOfDevicesByBus)(virConnectPtr conn, + const char *bus); + +typedef int (*virDrvNodeListDevicesByBus)(virConnectPtr conn, + const char *bus, + char **const names, + int maxnames); + +typedef virNodeDevicePtr (*virDrvNodeDeviceLookupByName)(virConnectPtr conn, + const char *name); + +typedef virNodeDevicePtr (*virDrvNodeDeviceLookupByKey)(virConnectPtr conn, + const char *key); + +typedef int (*virDrvNodeDeviceFree)(virNodeDevicePtr dev); + +typedef char * (*virDrvNodeDeviceDumpXML)(virNodeDevicePtr dev, + unsigned int flags); + +typedef virNodeDevicePtr (*virDrvNodeDeviceCreate)(virConnectPtr conn, + const char *xml); + +typedef int (*virDrvNodeDeviceDestroy)(virNodeDevicePtr dev);
Fine modulo the flags to be added ... [...]
+/** + * VIR_NODE_DEVICE_MAGIC: + * + * magic value used to protect the API when pointers to storage vol structures + * are passed down by the users. + */ +#define VIR_NODE_DEVICE_MAGIC 0xDEAD5679 +#define VIR_IS_NODE_DEVICE(obj) ((obj) && (obj)->magic==VIR_NODE_DEVICE_MAGIC) +#define VIR_IS_CONNECTED_NODE_DEVICE(obj) (VIR_IS_NODE_DEVICE(obj) && VIR_IS_CONNECT((obj)->conn)) +
Ah cool :-)
@@ -315,7 +325,7 @@ virInitialize(void) * * Handle an error at the connection level */ -static void +void virLibConnError(virConnectPtr conn, virErrorNumber error, const char *info) { const char *errmsg; @@ -336,7 +346,7 @@ virLibConnError(virConnectPtr conn, virErrorNumber error, const char *info) * * Handle an error at the connection level */ -static void +void virLibConnWarning(virConnectPtr conn, virErrorNumber error, const char *info) { const char *errmsg; @@ -454,6 +464,32 @@ virLibStorageVolError(virStorageVolPtr vol, virErrorNumber error, }
you really need to export them ? [...] for the libvirt.c entry points, looks fine but again flags should be added. In the mark them as "int flags ATTRIBUTE_UNUSED" in node_device_* since the code won't reference them ... yet. Hum ... we need the comment on the accessors, so they get extracted as part of the API when doing 'make rebuild' in docs/ added to the resulting XML, which then will provide the python accessors automatically I think.
+ + +const char *virNodeDeviceGetKey(virNodeDevicePtr dev) +{ + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL); + return dev->key; +} + +const char *virNodeDeviceGetName(virNodeDevicePtr dev) +{ + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL); + return dev->name; +} + +const char *virNodeDeviceGetParentKey(virNodeDevicePtr dev) +{ + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL); + return dev->parent_key; +} + +const char *virNodeDeviceGetBus(virNodeDevicePtr dev) +{ + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL); + return dev->bus_name; +} + +int virNodeDeviceNumOfCaps(virNodeDevicePtr dev) +{ + int ncaps = 0; + + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL); + while (dev->cap_names && dev->cap_names[ncaps] != NULL) + ++ncaps; + return ncaps; +} + +int virNodeDeviceListCaps(virNodeDevicePtr dev, + char **const names, + int maxnames) +{ + int n = 0; + + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL); + while (dev->cap_names && dev->cap_names[n] && n < maxnames) { + names[n] = dev->cap_names[n]; + n++; + } + return n; +}
diff --git a/src/libvirt_sym.version b/src/libvirt_sym.version index b8c470c..e214560 100644 --- a/src/libvirt_sym.version +++ b/src/libvirt_sym.version @@ -146,6 +146,19 @@ virStorageVolGetXMLDesc; virStorageVolGetPath;
+ virNodeNumOfDevices; + virNodeListDevices; + virNodeNumOfDevicesByCap; + virNodeListDevicesByCap; + virNodeNumOfDevicesByBus; + virNodeListDevicesByBus; + virNodeDeviceLookupByName; + virNodeDeviceLookupByKey; + virNodeDeviceFree; + virNodeDeviceGetXMLDesc; + virNodeDeviceCreate; + virNodeDeviceDestroy; + /* Symbols with __ are private only for use by the libvirtd daemon. They are not part of stable ABI @@ -165,6 +178,7 @@ __virGetNetwork; __virGetStoragePool; __virGetStorageVol; + __virGetNodeDevice;
__virEventRegisterImpl;
looks fine :-) Okay I didn't yet finished the review for the src/node_* new modules. I will try to get back to it tomorrow. So far that looks good to me, at least from a high level and API perspective ! thanks a lot ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, 2008-09-30 at 19:31 +0200, Daniel Veillard wrote:
Good to see that you seems to have the python bindings ready too !
Many python bindings are not really ready (in this patch). But I should have them in the next patch (Monday/Tuesday next week). I have complete java bindings for the current API, and will submit them once we agree upon it.
I wonder how many of them should be future-proofed by adding them a final flags argument too ... For example it may be useful to only lookup devices which are local to the machine, or the opposite only remote devices. We don't have to specify now flags values, but having the APIs ready is sufficient. The virNodeNum/virNodeListDevices devices can probably all share the same flags definitions when needed.
I agree. I'll add a flags arg to virNodeNum*/virNodeList*.
The LookupByName/LookupByKey may be able to use the same set. I wonder a bit about the key argument though, I assume it's something actually defined by the lower APIs (hal/devkit).
As long as the lookup keys are unique, I don't see why we'd want a flags arg for these functions. Currently the key is implementation- (HAL- or Devkit-) dependent, but I have questions about that (and for that matter, the name arg -- if it's unique, why have a separate key??). Dan B brings up some similar points, so I'll address this in my response to his post rather than here.
For virNodeDeviceCreate maybe a flags may be needed too, though the flexibility of the API is provided by the XML.
I think the XML should provide sufficient flexibility here. But let me know if you really want me to add a flag arg.
+int virNodeDeviceDestroy (virNodeDevicePtr dev); + +int virNodeDeviceFree (virNodeDevicePtr dev);
Maybe Destroy need flags too, for example to force (or not) destruction of devices which may be in use.
Ok, I'll add a flags arg to Destroy as well. FWIW, I'm not wild about the names virNodeDeviceCreate/Destroy. Don't "Attach" and "Detach" make more sense? If there are no objections, I'll change those in my next iteration (though I'll still leave them unimplemented).
@@ -332,6 +340,12 @@ skip_function = ( 'virCopyLastError', # Python API is called virGetLastError instead 'virConnectOpenAuth', # Python C code is manually written 'virDefaultErrorFunc', # Python virErrorFuncHandler impl calls this from C + 'virNodeDeviceGetKey', + 'virNodeDeviceGetName', + 'virNodeDeviceGetParentKey', + 'virNodeDeviceGetBus', + 'virNodeDeviceNumOfCaps', + 'virNodeDeviceListCaps', )
Strange how are the accessors supposed to work from python then ?
They don't. They're just here so you don't get errors building the Python bindings. I'll implement real bindings for these soon.
Hum, the libvirt_lxc really need to link to the device discovery libs ?
I was making host device enumeration work for ALL hypervisors (though of course the remote driver has a remote impl), so I think this is necessary. But that said, I'm still digesting Dan B's point about licensing issues, and I really have no clue about how lxc and openvz work with libvirt (do they have separate control daemons, like Xen, or are they more like QEMU, or ???).
virLibConnError(virConnectPtr conn, virErrorNumber error, const char *info) virLibConnWarning(virConnectPtr conn, virErrorNumber error, const char *info)
you really need to export them ?
Oh, uhhh, apparently not :-) (I did at one point, but must have removed that code.) I'll un-export them ...
Hum ... we need the comment on the accessors, so they get extracted as part of the API when doing 'make rebuild' in docs/ added to the resulting XML, which then will provide the python accessors automatically I think.
Will do. Thanks for the response. I'll implement the things discussed here (plus in Dan B's comments - another response coming) and submit a new patch on Monday or early Tuesday next week. Dave
On Fri, Oct 03, 2008 at 09:57:43AM -0400, David Lively wrote:
On Tue, 2008-09-30 at 19:31 +0200, Daniel Veillard wrote:
Good to see that you seems to have the python bindings ready too !
Many python bindings are not really ready (in this patch). But I should have them in the next patch (Monday/Tuesday next week).
I have complete java bindings for the current API, and will submit them once we agree upon it.
ahh, cool :-)
The LookupByName/LookupByKey may be able to use the same set. I wonder a bit about the key argument though, I assume it's something actually defined by the lower APIs (hal/devkit).
As long as the lookup keys are unique, I don't see why we'd want a flags arg for these functions.
Currently the key is implementation- (HAL- or Devkit-) dependent, but I have questions about that (and for that matter, the name arg -- if it's unique, why have a separate key??). Dan B brings up some similar points, so I'll address this in my response to his post rather than here.
okay
For virNodeDeviceCreate maybe a flags may be needed too, though the flexibility of the API is provided by the XML.
I think the XML should provide sufficient flexibility here. But let me know if you really want me to add a flag arg.
yes, for example if you want a blind asynch operation instead of waiting for the result.
+int virNodeDeviceDestroy (virNodeDevicePtr dev); + +int virNodeDeviceFree (virNodeDevicePtr dev);
Maybe Destroy need flags too, for example to force (or not) destruction of devices which may be in use.
Ok, I'll add a flags arg to Destroy as well. FWIW, I'm not wild about the names virNodeDeviceCreate/Destroy. Don't "Attach" and "Detach" make more sense? If there are no objections, I'll change those in my next iteration (though I'll still leave them unimplemented).
Create/Destroy has the good point of being similar to other libvirt functions.
Hum, the libvirt_lxc really need to link to the device discovery libs ?
I was making host device enumeration work for ALL hypervisors (though of course the remote driver has a remote impl), so I think this is necessary. But that said, I'm still digesting Dan B's point about licensing issues, and I really have no clue about how lxc and openvz work with libvirt (do they have separate control daemons, like Xen, or are they more like QEMU, or ???).
virLibConnError(virConnectPtr conn, virErrorNumber error, const char *info) virLibConnWarning(virConnectPtr conn, virErrorNumber error, const char *info)
you really need to export them ?
Oh, uhhh, apparently not :-) (I did at one point, but must have removed that code.) I'll un-export them ...
okay, thanks.
Hum ... we need the comment on the accessors, so they get extracted as part of the API when doing 'make rebuild' in docs/ added to the resulting XML, which then will provide the python accessors automatically I think.
Will do.
Thanks for the response. I'll implement the things discussed here (plus in Dan B's comments - another response coming) and submit a new patch on Monday or early Tuesday next week.
thanks a lot for pushing this forward :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Sep 30, 2008 at 12:17:27AM -0400, David Lively wrote:
Hi Daniel -
I'm not really ready to submit this yet (hence no [PATCH] in the subject), but it is functional enough to play with, mostly with the HAL-based driver, though the devkit-based one works too. In particular, I'm not yet happy with the code to gather capabilities and bus info (though gather_capabilities in node_device_hal.c is close to what I'm going for). Also, many of the details (which properties do we care about for which buses and capabilities?) that make this useful are missing, though I've provided a few guesses to get started. And finally, it's had very little testing at this point.
Thanks for posting this - its been very useful to see it in action even in its current state.
Configure --with-hal or --with-devkit to support one or both (if both are configured, HAL is preferred, provided we can talk to hald on dbus).
Makes sense, since from my little poking around DeviceKit itself is very incomplete at this stage in its life.
There are quite a few TODO's in the patch (mostly little things).
One other item for the list - Split stateful drivers out of libvirt.so, so they're only used by the daemon, and not apps linking to libvirt.so. This solves the licensing problem that HAL/DeviceKit introduce libvirt.so needs to be LGPL to allow closed-source apps to link to it. HAL/DeviceKit are both GPL or AFL licensed, by virtue of using DBus. Since LGPL is not AFL compatible, if we link to HAL/DeviceKit/DBus we do so under the GPL, and thus would prevent closed source apps using libvirt.so We don't want this, so we ned to make sure only the libvirtd daemon links to the GPL bits.
The larger TODO's are (in (my) priority order): * generalize gather_capabilities (node_device_hal.c) to work for HAL bus info as well * share the nodeDevices hash table among (non-remote) connections (perhaps store it in the virNodeDriver?) * register for HAL events to keep device info up-to-date * finish virsh, python support (for now there's enough to get devs/xml) * support more capability & bus properties (in HAL) * generalize gather_capabilities to work for gathering bus & capability info for both HAL & devkit drivers * figure out how devkit and HAL correlate, so we report device info consistently
This is looking like it'll be much harder than I had anticipated. DeviceKit as it stands is basically a front-end to udev providing a DBus API for accessing info udev has about devices. This is inherantly Linux-specific. The OS-agnostic APis are apparently ending up in the sub-system specific daemons like DeviceKit-Disks, but only the disk one exists at this time. So HAL is clearly the more portable option for a little while to come, but for Linux at least DeviceKit will (eventually) be the preferred way to access this kind of info. There are some interesting differences in the way info is provided. HAL has devices organized as a tree rooted at a 'computer' device. DeviceKit basically presents a flat list of devices, and provides no info correlating them. If you need to find out which PCI device is associated with a network device 'ethX', then we'd need to go outside the device kit API / dataset. This is rather painful :-( I don't think we want to have to manually create an entire hierarchy of all devices ourselves when using DeviceKit, but if we wanted to replicate the data from our HAL backend that's what we'd end up having todo. DeviceKit also seems to have removed the distinction between 'bus' and 'class'. Everything is now just a 'subsystem'. Some sub-systems are physical and can be mapped to what HAL called a 'bus' (pci, usb, etc), while othre sub-systems seem to be virtual and map to what HAL calls a 'class' (net, block, video, etc). So I'm thinking perhaps we need to simplify our modelling a little so its not so closely replicating HAL, getting rid of the distinct elements for 'class' and 'bus' and having a device simply have a 'subsystem'. And instead of having a complete tree hierarchy, have a specialized hierarchy. eg if we can identify a 'usb' or 'pci' device parent for a device, then include its name, but don't claim to provide a full hierarchy. The other interesting question, is what policy we should define for compatability. Do we absolutely need to have compatible keys & names for devices, whether using HAL vs DeviceKit, or can be just say that the format of a name is opaque and liable to change ? This has upgrade implications, for example, if we ship libvirt with a HAL backend in Fedora 10, and then switched it to the DeviceKit backend in Fedora 11, do we need to ensure consistent naming across the upgrade path. I don't know...
* register for devkit events to keep device info up-to-date
Dave
P.S. I'm afraid the patch is rather large, but remember to skip the generated files when looking it over:
configure.in | 97 ++++++ include/libvirt/libvirt.h | 78 +++++ include/libvirt/libvirt.h.in | 78 +++++ include/libvirt/virterror.h | 4 python/generator.py | 15 + python/libvir.c | 127 ++++++++ python/libvirt-python-api.xml | 17 + python/libvirt_wrap.h | 10 python/types.c | 15 + qemud/remote.c | 282 +++++++++++++++++++ qemud/remote_dispatch_localvars.h | 20 + qemud/remote_dispatch_proc_switch.h | 93 ++++++ qemud/remote_dispatch_prototypes.h | 11 qemud/remote_protocol.c | 220 +++++++++++++++ qemud/remote_protocol.h | 184 ++++++++++++ qemud/remote_protocol.x | 117 +++++++ src/Makefile.am | 19 + src/driver.h | 67 ++++ src/hash.c | 181 ++++++++++++ src/internal.h | 52 +++ src/libvirt.c | 528 +++++++++++++++++++++++++++++++++++\ - src/libvirt_sym.version | 14 src/node_device.c | 262 +++++++++++++++++ src/node_device.h | 38 ++ src/node_device_devkit.c | 299 ++++++++++++++++++++ src/node_device_hal.c | 475 ++++++++++++++++++++++++++++++++ src/remote_internal.c | 364 ++++++++++++++++++++++++ src/virsh.c | 80 +++++ src/virterror.c | 21 + 29 files changed, 3757 insertions(+), 11 deletions(-)
I've not had time for a full code review, so i'll just point out the things I noticed in a short glance through.
+ * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: David F. Lively <dlively@virtualiron.com> + */ + +#include <config.h> + +#include <unistd.h> +#include <errno.h> + +#include "internal.h" +#include "memory.h" + + +struct _stringv { + int len; + char **const base; + const int maxlen; +}; + +typedef struct _stringv stringv;
Perhaps I'm mis-undersatnding what this does, but isn't this similar to the virStringList class in internal.h ?
+static char *nodeDeviceDumpXML(virNodeDevicePtr dev, + unsigned int flags ATTRIBUTE_UNUSED) +{ + xmlBufferPtr buf = xmlBufferCreate(); + char *xml; + int i; + + if (buf == NULL) + return NULL; + + xmlBufferCCat(buf, "<device>\n <name>"); + xmlBufferCCat(buf, dev->name); + xmlBufferCCat(buf, "</name>\n <key>"); + xmlBufferCCat(buf, dev->key); + xmlBufferCCat(buf, "</key>\n");
This should use the libvirt buffer routines from buf.h which ensure you don't ignore return values indicating OOM like you have here :-) See buf.h, or HACKING file for some examples.
+ if (dev->parent_key) { + xmlBufferCCat(buf, " <parent>"); + xmlBufferCCat(buf, dev->parent_key); + xmlBufferCCat(buf, "</parent>\n"); + } + if (dev->bus) { + xmlBufferCCat(buf, " "); + xmlNodeDump(buf, NULL, dev->bus, 1, 1); + xmlBufferCCat(buf, "\n"); + } + if (dev->caps) { + for (i = 0; dev->caps[i]; i++) { + if (dev->caps[i]->parent == NULL) { + xmlBufferCCat(buf, " "); + xmlNodeDump(buf, NULL, dev->caps[i], 1, 1); + xmlBufferCCat(buf, "\n"); + } + } + } + xmlBufferCCat(buf, "</device>\n"); + + /* TODO: Can we avoid this copy? */ + xml = strdup((const char *)xmlBufferContent(buf));
Yes, you can with the libvirt buf.h APIs :-) Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, 2008-09-30 at 19:02 +0100, Daniel P. Berrange wrote:
On Tue, Sep 30, 2008 at 12:17:27AM -0400, David Lively wrote:
- Split stateful drivers out of libvirt.so, so they're only used by the daemon, and not apps linking to libvirt.so.
This solves the licensing problem that HAL/DeviceKit introduce libvirt.so needs to be LGPL to allow closed-source apps to link to it. HAL/DeviceKit are both GPL or AFL licensed, by virtue of using DBus. Since LGPL is not AFL compatible, if we link to HAL/DeviceKit/DBus we do so under the GPL, and thus would prevent closed source apps using libvirt.so We don't want this, so we ned to make sure only the libvirtd daemon links to the GPL bits.
Ok. I see. Will do.
* figure out how devkit and HAL correlate, so we report device info consistently
This is looking like it'll be much harder than I had anticipated.
Yeah, I struggled for a while trying to reconcile them. But devkit's not even close to complete yet, so I gave up. I'm assuming that, since there are a lot of HAL -> DevKit conversions to be done, some kind of (probably domain-specific) method(s) of correlating the two will eventually be described.
So I'm thinking perhaps we need to simplify our modelling a little so its not so closely replicating HAL, getting rid of the distinct elements for 'class' and 'bus' and having a device simply have a 'subsystem'. And instead of having a complete tree hierarchy, have a specialized hierarchy. eg if we can identify a 'usb' or 'pci' device parent for a device, then include its name, but don't claim to provide a full hierarchy.
Yes! I find the distinction between bus and capability to be pretty arbitrary anyway (being on a bus is a type of capability, IMO). So I'll drop the "bus" stuff, and combine bus capabilities with the other capabilities.
The other interesting question, is what policy we should define for compatability. Do we absolutely need to have compatible keys & names for devices, whether using HAL vs DeviceKit, or can be just say that the format of a name is opaque and liable to change ? This has upgrade implications, for example, if we ship libvirt with a HAL backend in Fedora 10, and then switched it to the DeviceKit backend in Fedora 11, do we need to ensure consistent naming across the upgrade path. I don't know...
Right. I was hoping to find a consistent set of keys/names to use, but gave up. (sysfs-paths seemed the most common denominator, but not all HAL devices have sysfs-paths, and I think the HAL names look more appropriate anyway.) I'm hoping that devkit will eventually add a property that either specifies or is convertible to a HAL name. Also, if names are really unique (as specified), then why have separate keys? I'd prefer dropping the Key functions and using Name as the key (as we do for storage pools, etc.).
+struct _stringv { + int len; + char **const base; + const int maxlen; +}; + +typedef struct _stringv stringv;
Perhaps I'm mis-undersatnding what this does, but isn't this similar to the virStringList class in internal.h ?
Well, it's a vector instead of a singly-linked list. I use it when I know the max length ahead of time (while the list is used in pool source discovery code that knows of no limit on the number of sources it will find). I could use a list for the former case, but that would require an extra copy of each name (plus unnecessary alloc/dealloc code for the list).
+static char *nodeDeviceDumpXML(virNodeDevicePtr dev, + unsigned int flags ATTRIBUTE_UNUSED) +{ + xmlBufferPtr buf = xmlBufferCreate(); ... + xmlBufferCCat(buf, "</key>\n"); This should use the libvirt buffer routines from buf.h which ensure you don't ignore return values indicating OOM like you have here :-) See buf.h, or HACKING file for some examples.
Note I call xmlNodeDump(buf, ...). I suppose the right thing to do is to add virBufNodeDump(...) to use libxml to dump into a virBuf ???
+ /* TODO: Can we avoid this copy? */ + xml = strdup((const char *)xmlBufferContent(buf));
Yes, you can with the libvirt buf.h APIs :-)
But if I have to have to call xmlNodeDump, I can't avoid it ... Thanks for the comments! Dave
On Fri, Oct 03, 2008 at 11:02:55AM -0400, David Lively wrote:
On Tue, 2008-09-30 at 19:02 +0100, Daniel P. Berrange wrote:
On Tue, Sep 30, 2008 at 12:17:27AM -0400, David Lively wrote:
- Split stateful drivers out of libvirt.so, so they're only used by the daemon, and not apps linking to libvirt.so.
This solves the licensing problem that HAL/DeviceKit introduce libvirt.so needs to be LGPL to allow closed-source apps to link to it. HAL/DeviceKit are both GPL or AFL licensed, by virtue of using DBus. Since LGPL is not AFL compatible, if we link to HAL/DeviceKit/DBus we do so under the GPL, and thus would prevent closed source apps using libvirt.so We don't want this, so we ned to make sure only the libvirtd daemon links to the GPL bits.
Ok. I see. Will do.
No, no don't do that ! I've already got this work underway, since it impacts much more than just the host device drivers. Just carry on with your existing way, and when its ready to merge it'll be easy to tweak the makefile to have it build in a suitable way.
So I'm thinking perhaps we need to simplify our modelling a little so its not so closely replicating HAL, getting rid of the distinct elements for 'class' and 'bus' and having a device simply have a 'subsystem'. And instead of having a complete tree hierarchy, have a specialized hierarchy. eg if we can identify a 'usb' or 'pci' device parent for a device, then include its name, but don't claim to provide a full hierarchy.
Yes! I find the distinction between bus and capability to be pretty arbitrary anyway (being on a bus is a type of capability, IMO).
So I'll drop the "bus" stuff, and combine bus capabilities with the other capabilities.
The other interesting question, is what policy we should define for compatability. Do we absolutely need to have compatible keys & names for devices, whether using HAL vs DeviceKit, or can be just say that the format of a name is opaque and liable to change ? This has upgrade implications, for example, if we ship libvirt with a HAL backend in Fedora 10, and then switched it to the DeviceKit backend in Fedora 11, do we need to ensure consistent naming across the upgrade path. I don't know...
Right. I was hoping to find a consistent set of keys/names to use, but gave up. (sysfs-paths seemed the most common denominator, but not all HAL devices have sysfs-paths, and I think the HAL names look more appropriate anyway.) I'm hoping that devkit will eventually add a property that either specifies or is convertible to a HAL name.
Also, if names are really unique (as specified), then why have separate keys? I'd prefer dropping the Key functions and using Name as the key (as we do for storage pools, etc.).
The ultimate goal with the storage was that 'key' was unique & consistent for the same storage on every host the storage was attached to, while 'name' could vary across machines. This distinction doesn't entirely apply to host devices, so perhaps droping key is OK - we could always re-add it later if we found a compelling need.
+static char *nodeDeviceDumpXML(virNodeDevicePtr dev, + unsigned int flags ATTRIBUTE_UNUSED) +{ + xmlBufferPtr buf = xmlBufferCreate(); ... + xmlBufferCCat(buf, "</key>\n"); This should use the libvirt buffer routines from buf.h which ensure you don't ignore return values indicating OOM like you have here :-) See buf.h, or HACKING file for some examples.
Note I call xmlNodeDump(buf, ...). I suppose the right thing to do is to add virBufNodeDump(...) to use libxml to dump into a virBuf ???
Ok, this actually makes me notice another problem - I'll reply to the original patch with details. In summary though, the internal data structure should not be using an xmlNodePtr as its canonical form, and thus there should not be a need for xmlNodeDump. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Sep 30, 2008 at 12:17:27AM -0400, David Lively wrote:
diff --git a/src/internal.h b/src/internal.h index d96504d..9a94f6d 100644 --- a/src/internal.h +++ b/src/internal.h @@ -292,6 +307,25 @@ struct _virStorageVol { };
+/** + * _virNodeDevice: + * + * Internal structure associated with a node device + */ +struct _virNodeDevice { + unsigned int magic; /* specific value to check */ + int refs; /* reference count */ + virConnectPtr conn; /* pointer back to the connection */ + char *key; /* device key */ + char *name; /* device name */ + char *parent_key; /* parent device */ + char *bus_name; /* (optional) bus name */ + xmlNodePtr bus; /* (optional) bus descriptor */ + char **cap_names; /* capability names (null-term) */ + xmlNodePtr *caps; /* capability descs (null-term) */ +};
This is not the right way to maintain the internal representation of devices. The model we follow is to have 2, sometimes 3 structs to represent an object. I'll summarize in terms of domain objects - virDomainPtr - the public opaque struct representing a domain the internal struct is defined in internal.h and merely contains reference counting, connection pointer, and any unique keys (ID, name, UUID). - virDomainObjPtr - the internal struct representing the state of a domain object. Not all drivers use us - only stateful drivers do. It is used to track state such as guest PID, logfile, running state, and a pointer to the live and inactive configs This is done in domain_conf.h/.c - virDomainDefPtr - the internal struct representing the canonical configuration of a domain. This is a direct mapping of the XML into a series of structs. This is done in domain_conf.h/.c So, in the context for host devices we need a few changes. In the internal.h file, you should only store the key/name attributes. There should then be a separate file handling configuration parsing and formatting, node_device_conf.h/.c. There is probably no need for a state object, so skip virNodeDeviceObjPtr, and just create a virNodeDeviceDefPtr representing the XML format as a series of structs/unions, etc. See virDomainDefPtr for a good example. This should not store any xmlNodePtr instances - it should be all done as explicit struct/enum fields The node_device_conf.c file should at mimium have 2 methods, one for converting an XML document into a virNodeDeviceDefPtr object, and one for the converting a virNodeDeviceObjPtr back into an XML document. Follow the existing API naming / method signatures that are seen in domain_conf.h / network_conf.h for current 'best practice' Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Okay, I see what you mean. I'll create a virNodeDeviceDefPtr and follow the established pattern. I'm really trying to avoid creating a struct/union that must be extended every time we support a new capability. I struggled quite a bit with the right representation for capabilities and bus details.. At one point, I had my own (general-purpose; i.e., not specific to any type of capability) virNodeDeviceCapabilities type, but it looked so much like a DOM tree that just using a xmlNode seemed like the best choice. Are you suggesting creating a struct for each kind of currently-supported capability, or are you suggesting creating a single struct that's general enough to represent all capabilities? Dave On Fri, 2008-10-03 at 16:23 +0100, Daniel P. Berrange wrote:
There should then be a separate file handling configuration parsing and formatting, node_device_conf.h/.c. There is probably no need for a state object, so skip virNodeDeviceObjPtr, and just create a virNodeDeviceDefPtr representing the XML format as a series of structs/unions, etc. See virDomainDefPtr for a good example. This should not store any xmlNodePtr instances - it should be all done as explicit struct/enum fields
The node_device_conf.c file should at mimium have 2 methods, one for converting an XML document into a virNodeDeviceDefPtr object, and one for the converting a virNodeDeviceObjPtr back into an XML document. Follow the existing API naming / method signatures that are seen in domain_conf.h / network_conf.h for current 'best practice'
Daniel
On Fri, Oct 03, 2008 at 01:20:35PM -0400, David Lively wrote:
Okay, I see what you mean. I'll create a virNodeDeviceDefPtr and follow the established pattern.
I'm really trying to avoid creating a struct/union that must be extended every time we support a new capability. I struggled quite a bit with the right representation for capabilities and bus details.. At one point, I had my own (general-purpose; i.e., not specific to any type of capability) virNodeDeviceCapabilities type, but it looked so much like a DOM tree that just using a xmlNode seemed like the best choice.
Are you suggesting creating a struct for each kind of currently-supported capability, or are you suggesting creating a single struct that's general enough to represent all capabilities?
I'm suggesting a something that is specific for each capability. Now if we were to support all metadata that HAL exposes this would be a huge job. But I don't believe we should be exposing all the metadata that HAL has, because in the future this XML format has to work with DeviceKit which is basically exposing UDev metadata, and VMWare's APIs which expose hardware info in their own way. Thus, IMHO, we should expose specific capabilities we know we care about, for specific sub-substems, and not try to handle the entire of HAL. A good starting point would be - PCI, domain, bus, slot, function, and vendor/product - USB, bus, device and vendor/product Basically same info required for attaching the device to a domain, thus matching the struct virDomainHostdevDefPtr in domain_conf.h - NIC, name & mac address - Block, name and host adapter - Host adapter, name For these also have a way to link to the parent device associated with them (ie the PCI/USB device providing them). That would basically be enough for use of the existing domain/storage and network APIs which is what we need this data for, and this minimal info should be satisifiable with VMWare's APIs, and DeviceKit. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
I definitely wasn't planning on covering all of HAL :-) I assume you weren't planning on exposing these capability-specific representations in the public API. Right? (If not, ignore the rest of this ...) So I guess I don't see the value of having these cap-specific internal representations. I keep a string array of the cap names for ListDevicesByCap, but other than that, the capability data is used only by virNodeDeviceGetXMLDesc(). So it seems natural to represent it in a form that can easily be converted to XML, and that can represent all the XML we'll need to output (like xmlNode). Otherwise, we are forced to write more capability-specific code, with no particular advantage (no stronger typing guarantees if we don't expose specific cap types). Dave P.S. I do think it would be useful to have access to capability details other than GetXMLDesc. Perhaps: const char *virNodeDeviceCapabilityProperty(virNodeDevicePtr dev, const char *cap, const char *prop) but note this exposes them only in a general (property / value) way, and is quite easily implemented with a xmlNode representation. On Fri, 2008-10-03 at 18:41 +0100, Daniel P. Berrange wrote:
On Fri, Oct 03, 2008 at 01:20:35PM -0400, David Lively wrote:
Okay, I see what you mean. I'll create a virNodeDeviceDefPtr and follow the established pattern.
I'm really trying to avoid creating a struct/union that must be extended every time we support a new capability. I struggled quite a bit with the right representation for capabilities and bus details.. At one point, I had my own (general-purpose; i.e., not specific to any type of capability) virNodeDeviceCapabilities type, but it looked so much like a DOM tree that just using a xmlNode seemed like the best choice.
Are you suggesting creating a struct for each kind of currently-supported capability, or are you suggesting creating a single struct that's general enough to represent all capabilities?
I'm suggesting a something that is specific for each capability. Now if we were to support all metadata that HAL exposes this would be a huge job. But I don't believe we should be exposing all the metadata that HAL has, because in the future this XML format has to work with DeviceKit which is basically exposing UDev metadata, and VMWare's APIs which expose hardware info in their own way.
Thus, IMHO, we should expose specific capabilities we know we care about, for specific sub-substems, and not try to handle the entire of HAL.
A good starting point would be
- PCI, domain, bus, slot, function, and vendor/product - USB, bus, device and vendor/product
Basically same info required for attaching the device to a domain, thus matching the struct virDomainHostdevDefPtr in domain_conf.h
- NIC, name & mac address - Block, name and host adapter - Host adapter, name
For these also have a way to link to the parent device associated with them (ie the PCI/USB device providing them).
That would basically be enough for use of the existing domain/storage and network APIs which is what we need this data for, and this minimal info should be satisifiable with VMWare's APIs, and DeviceKit.
Regards, Daniel
On Fri, Oct 03, 2008 at 02:35:06PM -0400, David Lively wrote:
I definitely wasn't planning on covering all of HAL :-)
I assume you weren't planning on exposing these capability-specific representations in the public API. Right? (If not, ignore the rest of this ...)
Not at this time. For the forseeable future I expect the XML format to be the only publically exposed representation of configuration. A long time down the road when we're confident the representation is long term sustainable & correct, we might consider exposing the structs, but that is a long way off.
So I guess I don't see the value of having these cap-specific internal representations. I keep a string array of the cap names for ListDevicesByCap, but other than that, the capability data is used only by virNodeDeviceGetXMLDesc(). So it seems natural to represent it in a form that can easily be converted to XML, and that can represent all the XML we'll need to output (like xmlNode). Otherwise, we are forced to write more capability-specific code, with no particular advantage (no stronger typing guarantees if we don't expose specific cap types).
The idea is that by having formal internal representations it makes it easier for internal driver code to work with it in a gaurenteed consistent fashion. While the structs may only be used by your specific driver for the intiial commit, experiance has shown our needs evolve over time and we'll probably end up with other internal code wanting it. We previously had each hypervisor driver using an adhoc representation internally for domain configs, and it just wasn't sustainable our internal usage evolved.
P.S. I do think it would be useful to have access to capability details other than GetXMLDesc. Perhaps: const char *virNodeDeviceCapabilityProperty(virNodeDevicePtr dev, const char *cap, const char *prop) but note this exposes them only in a general (property / value) way, and is quite easily implemented with a xmlNode representation.
I'm wary of exposing sub-sets of the XML docs in simple key/value pairs because our XML formats have tended to expand over time, so things that started off key/value pairs gained extra attributes/child elements, all of which are desired. It is simple enough for applications to wrap in a property 'getter' using XPath on the XML doc if desired - virt-manager does this internally for many things. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Hi Daniel - I'm implementing virNodeDeviceDef, as suggested, but I just noticed something that really bothers me. In a nutshell, it's that the current implementation of this scheme unnecessarily increases the (time) complexity of O(1) operations to O(n). For example, take virDomainGetXMLDesc(). One would think dumping the XML descriptor for a domain object we have in hand would be O(1), but in fact, it's O(n) (where n is the number of domains with the same driver), because qemudDomainDumpXML must find the DomainObjPtr (from which it can get the def). I suppose this lookup could be made O(log n) if the domains were sorted by UUID on the list, but the fact remains that lookup could be eliminated entirely. Perhaps it's just a matter of allowing the "public" objects (virDomain, virNetwork, etc.) to hold a pointer to their optional (never used by remote driver, for example) private state objects. So I guess I'm suggesting adding a void * to each of the public objects, which drivers can use for this purpose. I'll go ahead and do this for NodeDevices in the next incarnation of this patch to show you what I mean. (It won't be hard go to the conventional lookup impl if this turns out to be a bad idea.) Dave On Fri, 2008-10-03 at 16:23 +0100, Daniel P. Berrange wrote:
On Tue, Sep 30, 2008 at 12:17:27AM -0400, David Lively wrote:
diff --git a/src/internal.h b/src/internal.h index d96504d..9a94f6d 100644 --- a/src/internal.h +++ b/src/internal.h @@ -292,6 +307,25 @@ struct _virStorageVol { };
+/** + * _virNodeDevice: + * + * Internal structure associated with a node device + */ +struct _virNodeDevice { + unsigned int magic; /* specific value to check */ + int refs; /* reference count */ + virConnectPtr conn; /* pointer back to the connection */ + char *key; /* device key */ + char *name; /* device name */ + char *parent_key; /* parent device */ + char *bus_name; /* (optional) bus name */ + xmlNodePtr bus; /* (optional) bus descriptor */ + char **cap_names; /* capability names (null-term) */ + xmlNodePtr *caps; /* capability descs (null-term) */ +};
This is not the right way to maintain the internal representation of devices.
The model we follow is to have 2, sometimes 3 structs to represent an object. I'll summarize in terms of domain objects
- virDomainPtr - the public opaque struct representing a domain the internal struct is defined in internal.h and merely contains reference counting, connection pointer, and any unique keys (ID, name, UUID).
- virDomainObjPtr - the internal struct representing the state of a domain object. Not all drivers use us - only stateful drivers do. It is used to track state such as guest PID, logfile, running state, and a pointer to the live and inactive configs This is done in domain_conf.h/.c
- virDomainDefPtr - the internal struct representing the canonical configuration of a domain. This is a direct mapping of the XML into a series of structs. This is done in domain_conf.h/.c
So, in the context for host devices we need a few changes. In the internal.h file, you should only store the key/name attributes.
There should then be a separate file handling configuration parsing and formatting, node_device_conf.h/.c. There is probably no need for a state object, so skip virNodeDeviceObjPtr, and just create a virNodeDeviceDefPtr representing the XML format as a series of structs/unions, etc. See virDomainDefPtr for a good example. This should not store any xmlNodePtr instances - it should be all done as explicit struct/enum fields
The node_device_conf.c file should at mimium have 2 methods, one for converting an XML document into a virNodeDeviceDefPtr object, and one for the converting a virNodeDeviceObjPtr back into an XML document. Follow the existing API naming / method signatures that are seen in domain_conf.h / network_conf.h for current 'best practice'
Daniel
On Thu, Oct 09, 2008 at 12:25:18PM -0400, David Lively wrote:
Hi Daniel - I'm implementing virNodeDeviceDef, as suggested, but I just noticed something that really bothers me. In a nutshell, it's that the current implementation of this scheme unnecessarily increases the (time) complexity of O(1) operations to O(n). For example, take virDomainGetXMLDesc(). One would think dumping the XML descriptor for a domain object we have in hand would be O(1), but in fact, it's O(n) (where n is the number of domains with the same driver), because qemudDomainDumpXML must find the DomainObjPtr (from which it can get the def). I suppose this lookup could be made O(log n) if the domains were sorted by UUID on the list, but the fact remains that lookup could be eliminated entirely.
Sure the lookup algorithm is O(n), but have you actually got real world benchmarks showing this is the serious bottleneck? In the XML dump example, the time to lookup the object is likely dwarfed by the time to generate the XML doc, or to talk to the QEMU monitor. IMHO, optimizing this is overkill. Worst case we can just hash on the UUID if it ever provdes a problem.
Perhaps it's just a matter of allowing the "public" objects (virDomain, virNetwork, etc.) to hold a pointer to their optional (never used by remote driver, for example) private state objects. So I guess I'm suggesting adding a void * to each of the public objects, which drivers can use for this purpose. I'll go ahead and do this for NodeDevices in the next incarnation of this patch to show you what I mean. (It won't be hard go to the conventional lookup impl if this turns out to be a bad idea.)
No please don't do that. We really need to keep the hard separation between the public vir{Domain,Network,Storage,Node}Ptr objects, and the internal objects completely separated. The only association should be via the various unique keys - ID, Name & UUID as appropriate. The public objects do not neccessarily have the same lifecycle as the internal object - eg, an app can hold onto the public object even after the internal representation has been killed off. Keeping a reference from the public to private objects, means we also need to have a reverse references from priate to public objects, so we can kill off the references when objects go away. This also means we need to have complex locking from the internal drivers to the public objects which for thread safety which I really do not want. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, 2008-10-09 at 18:01 +0100, Daniel P. Berrange wrote:
On Thu, Oct 09, 2008 at 12:25:18PM -0400, David Lively wrote:
Hi Daniel - I'm implementing virNodeDeviceDef, as suggested, but I just noticed something that really bothers me. In a nutshell, it's that the current implementation of this scheme unnecessarily increases the (time) complexity of O(1) operations to O(n). For example, take virDomainGetXMLDesc(). One would think dumping the XML descriptor for a domain object we have in hand would be O(1), but in fact, it's O(n) (where n is the number of domains with the same driver), because qemudDomainDumpXML must find the DomainObjPtr (from which it can get the def). I suppose this lookup could be made O(log n) if the domains were sorted by UUID on the list, but the fact remains that lookup could be eliminated entirely.
Sure the lookup algorithm is O(n), but have you actually got real world benchmarks showing this is the serious bottleneck? In the XML
Well ... uhh ... no. :-o
dump example, the time to lookup the object is likely dwarfed by the time to generate the XML doc, or to talk to the QEMU monitor. IMHO, optimizing this is overkill. Worst case we can just hash on the UUID if it ever provdes a problem.
Okay, I'll buy that. And (now) I see what you mean about the differing object lifetime. ... In fact (perhaps I shouldn't be admitting this publicly), your whole public-obj/Obj scheme now finally makes sense to me! The public objs are really just a kind of handle to the private Objs, and they exist independently of one another. public objs belong to the apps, private Objs to the appropriate driver. And (now that I look at the Xen driver), I finally understand that there are stateless drivers. In fact, both the devkit and HAL node-dev drivers need to be stateful (since the device name isn't a reasonable handle for either devkit or HAL devices), so I'm creating a virNodeDeviceObj as well as a virNodeDeviceDef. Dave
On Thu, Oct 09, 2008 at 02:29:18PM -0400, David Lively wrote:
On Thu, 2008-10-09 at 18:01 +0100, Daniel P. Berrange wrote:
dump example, the time to lookup the object is likely dwarfed by the time to generate the XML doc, or to talk to the QEMU monitor. IMHO, optimizing this is overkill. Worst case we can just hash on the UUID if it ever provdes a problem.
Okay, I'll buy that. And (now) I see what you mean about the differing object lifetime.
... In fact (perhaps I shouldn't be admitting this publicly), your whole public-obj/Obj scheme now finally makes sense to me! The public objs are really just a kind of handle to the private Objs, and they exist independently of one another. public objs belong to the apps, private Objs to the appropriate driver. And (now that I look at the Xen driver), I finally understand that there are stateless drivers.
I think its clear that the major thing lacking here is good documentation on the structure / internals of libvirt & its object/driver modelling. Until very recently it was pretty much every driver for itself. With the introduction of unified internal objects, we do now have some formal data structure, beyond just the driver.h API contracts. Until we document this its not surprising that people find it all rather opaque to understand Time for me to add a section to the website docs covering internal architecture in more detail...
In fact, both the devkit and HAL node-dev drivers need to be stateful (since the device name isn't a reasonable handle for either devkit or HAL devices), so I'm creating a virNodeDeviceObj as well as a virNodeDeviceDef.
Great, that meshes with my mental model of things Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Ok, here's my substantially-reworked node device enumeration patch, this time done with a proper understanding of the public-obj/Obj/Def model :-) as last discussed here: https://www.redhat.com/archives/libvir-list/2008-September/msg00398.html I've broken it into the following pieces: 1-public-api additions to the public API 2-internal-api additions to the internal API 3-local-node-drivers the HAL & DeviceKit implementations 4-remote-node-driver the remote driver 5-virsh-support virsh support 6-python-bindings python bindings Big differences from the last submission: * public-obj/Obj/Def object model finally understood :-) * capabilities structure now struct-ified, handled like other Def bits * using newfangled array-based lists for NodeDeviceObj's * added flags args to various public APIs (as suggested by Dan V) * "bus" folded into capabilities (as discussed w/Dan B) * device "key" no longer exists - name is unique on node Some pieces are still incomplete, but I thought it would be better to post what I have since it's useful "as is". Here's what I know of that's left to do: * finish Python bindings (will get to Real Soon Now) * submit Java bindings (I have them, and have been using them) * implement virNodeDeviceCreate/Destroy (I have no plans for these) * subscribe to HAL events & update dev state from them (next for me?) * implement pci_bus/usb_bus capabilities (for PCI/USB controllers) * DeviceKit implementation is barely a proof of concept * need to resolve naming & other issues (see https://www.redhat.com/archives/libvir-list/2008-September/msg00430.html * ... and then fill in impl (no hurry; devkit immature now) Dave
This patch contains the public API additions.
On Tue, Oct 21, 2008 at 01:49:19PM -0400, David Lively wrote:
This patch contains the public API additions.
the libvirt.h part looks just fine at this point for me. I'm just wondering a bit about some of the naming "node driver" are devkit and Hal really "node drivers", then sound more like "device watcher". Maybe the error file should state VIR_FROM_DEVWATCH instead of VIR_FROM_NODE then the names we pick for the internal symbols can be changed We will need to augment the terminology section at some point but I'm not sure what's the best way to describe those new internal APIs, "Node driver" doesn't really fit IMHO, "Device watcher" is a bit better but doesn't feel great either. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
I'm not crazy about "node driver" either (it has drifted somewhat from its original intention, as I understood that many of the "node" operations may be hypervisor-dependent). Ben Guthro suggested "Device Monitor" which seems just right to me. I *think* it's clear that we're monitoring the node's devices (and not virtual ones). Dave On Tue, 2008-10-28 at 14:01 +0100, Daniel Veillard wrote:
On Tue, Oct 21, 2008 at 01:49:19PM -0400, David Lively wrote:
This patch contains the public API additions.
the libvirt.h part looks just fine at this point for me. I'm just wondering a bit about some of the naming "node driver" are devkit and Hal really "node drivers", then sound more like "device watcher". Maybe the error file should state VIR_FROM_DEVWATCH instead of VIR_FROM_NODE then the names we pick for the internal symbols can be changed
We will need to augment the terminology section at some point but I'm not sure what's the best way to describe those new internal APIs, "Node driver" doesn't really fit IMHO, "Device watcher" is a bit better but doesn't feel great either.
Daniel
On Tue, Oct 28, 2008 at 10:47:21AM -0400, David Lively wrote:
I'm not crazy about "node driver" either (it has drifted somewhat from its original intention, as I understood that many of the "node" operations may be hypervisor-dependent).
Ben Guthro suggested "Device Monitor" which seems just right to me. I *think* it's clear that we're monitoring the node's devices (and not virtual ones).
Ah yes :-) "Device Monitor" sounds better ! What about VIR_FROM_DEVMONITOR then in the API ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
This patch contains the internal API additions.
On Tue, Oct 21, 2008 at 01:54:13PM -0400, David Lively wrote:
This patch contains the internal API additions.
Looks fine to me, just a couple of questions:
+ // TODO: Implement PCI bus devs + // TODO: Implement USB bus devs
Any reason it's not in ? I would expect HAL to report them, no ?
+ +// TODO: virNodeDeviceDefParseString/File/Node for virNodeDeviceCreate
No sure, could you explain a bit ? thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, 2008-10-28 at 14:13 +0100, Daniel Veillard wrote:
On Tue, Oct 21, 2008 at 01:54:13PM -0400, David Lively wrote:
This patch contains the internal API additions.
Looks fine to me, just a couple of questions:
+ // TODO: Implement PCI bus devs + // TODO: Implement USB bus devs
Any reason it's not in ? I would expect HAL to report them, no ?
I posted a new patch set yesterday that (among other things) removed the PCI_BUS and USB_BUS devices. Currently HAL doesn't report them as anything special, e.g.: udi = '/org/freedesktop/Hal/devices/pci_8086_27cc' info.linux.driver = 'ehci_hcd' (string) info.parent = '/org/freedesktop/Hal/devices/computer' (string) info.product = '82801G (ICH7 Family) USB2 EHCI Controller' (string) info.subsystem = 'pci' (string) info.udi = '/org/freedesktop/Hal/devices/pci_8086_27cc' (string) info.vendor = 'Intel Corporation' (string) linux.hotplug_type = 2 (0x2) (int) linux.subsystem = 'pci' (string) linux.sysfs_path = '/sys/devices/pci0000:00/0000:00:1d.7' (string) pci.device_class = 12 (0xc) (int) pci.device_protocol = 32 (0x20) (int) pci.device_subclass = 3 (0x3) (int) pci.linux.sysfs_path = '/sys/devices/pci0000:00/0000:00:1d.7' (string) pci.product = '82801G (ICH7 Family) USB2 EHCI Controller' (string) pci.product_id = 10188 (0x27cc) (int) pci.subsys_product_id = 22594 (0x5842) (int) pci.subsys_vendor = 'Intel Corporation' (string) pci.subsys_vendor_id = 32902 (0x8086) (int) pci.vendor = 'Intel Corporation' (string) pci.vendor_id = 32902 (0x8086) (int) udi = '/org/freedesktop/Hal/devices/pci_8086_244e' info.parent = '/org/freedesktop/Hal/devices/computer' (string) info.product = '82801 PCI Bridge' (string) info.subsystem = 'pci' (string) info.udi = '/org/freedesktop/Hal/devices/pci_8086_244e' (string) info.vendor = 'Intel Corporation' (string) linux.hotplug_type = 2 (0x2) (int) linux.subsystem = 'pci' (string) linux.sysfs_path = '/sys/devices/pci0000:00/0000:00:1e.0' (string) pci.device_class = 6 (0x6) (int) pci.device_protocol = 1 (0x1) (int) pci.device_subclass = 4 (0x4) (int) pci.linux.sysfs_path = '/sys/devices/pci0000:00/0000:00:1e.0' (string) pci.product = '82801 PCI Bridge' (string) pci.product_id = 9294 (0x244e) (int) pci.subsys_product_id = 0 (0x0) (int) pci.subsys_vendor_id = 0 (0x0) (int) pci.vendor = 'Intel Corporation' (string) pci.vendor_id = 32902 (0x8086) (int) So at this point, I just report them as PCI devices, though of course the PCI bridge will be the parent of PCI devices on that bus, and the USB controller will be the parent of USB devices controlled by it. Any client code that needs to recognize them specifically as PCI or USB bus controllers can use the PCI device attributes for that. Hmmm ... currently I expose the product & vendor info for PCI devices, along with their bus addressing info. But perhaps I should also expose their device_class, protocol, and subclass??
+ +// TODO: virNodeDeviceDefParseString/File/Node for virNodeDeviceCreate
Currently I have no need to parse a NodeDevice definition. But virNodeDeviceCreate will need to do that, once it's implemented. (I don't have any NPIV NICs or other such toys to play with right now. Daniel B says he'll implement Create/Destroy since he does.) I also thought it would be better to postpone the parser work until needed since the node definition seems likely to change as we work out the details here. Dave
On Tue, Oct 28, 2008 at 11:32:03AM -0400, David Lively wrote:
On Tue, 2008-10-28 at 14:13 +0100, Daniel Veillard wrote:
On Tue, Oct 21, 2008 at 01:54:13PM -0400, David Lively wrote:
This patch contains the internal API additions.
Looks fine to me, just a couple of questions:
+ // TODO: Implement PCI bus devs + // TODO: Implement USB bus devs
Any reason it's not in ? I would expect HAL to report them, no ?
I posted a new patch set yesterday that (among other things) removed the PCI_BUS and USB_BUS devices. Currently HAL doesn't report them as anything special, e.g.:
udi = '/org/freedesktop/Hal/devices/pci_8086_27cc' info.linux.driver = 'ehci_hcd' (string) info.parent = '/org/freedesktop/Hal/devices/computer' (string) info.product = '82801G (ICH7 Family) USB2 EHCI Controller' (string) info.subsystem = 'pci' (string) info.udi = '/org/freedesktop/Hal/devices/pci_8086_27cc' (string) info.vendor = 'Intel Corporation' (string) linux.hotplug_type = 2 (0x2) (int) linux.subsystem = 'pci' (string) linux.sysfs_path = '/sys/devices/pci0000:00/0000:00:1d.7' (string) pci.device_class = 12 (0xc) (int) pci.device_protocol = 32 (0x20) (int) pci.device_subclass = 3 (0x3) (int) pci.linux.sysfs_path = '/sys/devices/pci0000:00/0000:00:1d.7' (string) pci.product = '82801G (ICH7 Family) USB2 EHCI Controller' (string) pci.product_id = 10188 (0x27cc) (int) pci.subsys_product_id = 22594 (0x5842) (int) pci.subsys_vendor = 'Intel Corporation' (string) pci.subsys_vendor_id = 32902 (0x8086) (int) pci.vendor = 'Intel Corporation' (string) pci.vendor_id = 32902 (0x8086) (int)
udi = '/org/freedesktop/Hal/devices/pci_8086_244e' info.parent = '/org/freedesktop/Hal/devices/computer' (string) info.product = '82801 PCI Bridge' (string) info.subsystem = 'pci' (string) info.udi = '/org/freedesktop/Hal/devices/pci_8086_244e' (string) info.vendor = 'Intel Corporation' (string) linux.hotplug_type = 2 (0x2) (int) linux.subsystem = 'pci' (string) linux.sysfs_path = '/sys/devices/pci0000:00/0000:00:1e.0' (string) pci.device_class = 6 (0x6) (int) pci.device_protocol = 1 (0x1) (int) pci.device_subclass = 4 (0x4) (int) pci.linux.sysfs_path = '/sys/devices/pci0000:00/0000:00:1e.0' (string) pci.product = '82801 PCI Bridge' (string) pci.product_id = 9294 (0x244e) (int) pci.subsys_product_id = 0 (0x0) (int) pci.subsys_vendor_id = 0 (0x0) (int) pci.vendor = 'Intel Corporation' (string) pci.vendor_id = 32902 (0x8086) (int)
So at this point, I just report them as PCI devices, though of course the PCI bridge will be the parent of PCI devices on that bus, and the USB controller will be the parent of USB devices controlled by it. Any client code that needs to recognize them specifically as PCI or USB bus controllers can use the PCI device attributes for that.
Hmmm ... currently I expose the product & vendor info for PCI devices, along with their bus addressing info. But perhaps I should also expose their device_class, protocol, and subclass??
+ +// TODO: virNodeDeviceDefParseString/File/Node for virNodeDeviceCreate
Currently I have no need to parse a NodeDevice definition. But virNodeDeviceCreate will need to do that, once it's implemented. (I don't have any NPIV NICs or other such toys to play with right now. Daniel B says he'll implement Create/Destroy since he does.) I also thought it would be better to postpone the parser work until needed since the node definition seems likely to change as we work out the details here.
Okay, thanks for the clarification, and sorry for the delay, as you can guess I didn't yet reached the end of the folder <grin/> ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
This patch contains the actual HAL- and DeviceKit-based local "node" drivers.
This patch contains the remote impl of the node driver.
This patch implements virsh support. It's pretty sparse right now, and I'm not wild about the command names I chose. I welcome suggestions (i.e., command names / args) for completing (or changing) this.
This patch implements the python bindings, rather lamely. I'll submit a new version with a proper NodeDevice object soon ...
Ok, here are proper python bindings to the node device enumeration functions and objects.
On Tue, Oct 21, 2008 at 01:45:47PM -0400, David Lively wrote:
Ok, here's my substantially-reworked node device enumeration patch, this time done with a proper understanding of the public-obj/Obj/Def model :-) as last discussed here: https://www.redhat.com/archives/libvir-list/2008-September/msg00398.html
I've broken it into the following pieces: 1-public-api additions to the public API 2-internal-api additions to the internal API 3-local-node-drivers the HAL & DeviceKit implementations 4-remote-node-driver the remote driver 5-virsh-support virsh support 6-python-bindings python bindings
Big differences from the last submission: * public-obj/Obj/Def object model finally understood :-) * capabilities structure now struct-ified, handled like other Def bits
I like the way this bit turned out - makes it very clear what properties we are exporting in the XML as part of our API/ABI gaurentee.
* using newfangled array-based lists for NodeDeviceObj's
The individual capabilities within a device are still using a linked list, though that's not a critical problem from the thread safety point of view.
* added flags args to various public APIs (as suggested by Dan V) * "bus" folded into capabilities (as discussed w/Dan B)
Yes, this looks much nicer too.
* device "key" no longer exists - name is unique on node
Some pieces are still incomplete, but I thought it would be better to post what I have since it's useful "as is". Here's what I know of that's left to do: * finish Python bindings (will get to Real Soon Now) * submit Java bindings (I have them, and have been using them) * implement virNodeDeviceCreate/Destroy (I have no plans for these)
No need to worry about this - I'd like to use them to implement the creation/deletion of NPIV virtual HBAs eventually.
* subscribe to HAL events & update dev state from them (next for me?) * implement pci_bus/usb_bus capabilities (for PCI/USB controllers)
While on the subject of USB, I've just noticed that HAL seems to expose 2 devices for every USB device - 'usb' and 'usb_device', which we're mapping into just a 'usb' capability. Unless there's compelling reason to have both we might consider just filtering out one of the two.
* DeviceKit implementation is barely a proof of concept
Noticed one problem - on my build it refuses to enumerate devices if you pass in a NULL for subsystem name list. I made a really quick & dirty hack to just try it out @@ -300,13 +301,18 @@ static int devkitNodeDriverStartup(void) } /* Populate with known devices */ - devs = devkit_client_enumerate_by_subsystem(devkit_client, NULL, &err); - if (err) { - DEBUG0("devkit_client_enumerate_by_subsystem failed"); - devs = NULL; - goto failure; + for (i = 0 ; i < ARRAY_CARDINALITY(caps_tbl) ; i++) { + const char *caps[] = { caps_tbl[i].cap_name, NULL }; + devs = devkit_client_enumerate_by_subsystem(devkit_client, + caps, + &err); + if (err) { + DEBUG0("devkit_client_enumerate_by_subsystem failed"); + devs = NULL; + goto failure; + } + g_list_foreach(devs, dev_create, devkit_client); } - g_list_foreach(devs, dev_create, devkit_client); driverState->privateData = devkit_client;
* need to resolve naming & other issues (see https://www.redhat.com/archives/libvir-list/2008-September/msg00430.html * ... and then fill in impl (no hurry; devkit immature now)
I'm still wondering if it is worth us santizing the device names ourselves somehow, though it might end up being rather a large job. Will have to think some more about it. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, 2008-10-23 at 13:53 +0100, Daniel P. Berrange wrote:
On Tue, Oct 21, 2008 at 01:45:47PM -0400, David Lively wrote:
* using newfangled array-based lists for NodeDeviceObj's
The individual capabilities within a device are still using a linked list, though that's not a critical problem from the thread safety point of view.
Right. This was intentional, since there are no per-capability operations (and hence no need for per-capability locking granularity).
* subscribe to HAL events & update dev state from them (next for me?)
BTW, I'm working on this now. Should have something to submit in another day or two.
* implement pci_bus/usb_bus capabilities (for PCI/USB controllers)
While on the subject of USB, I've just noticed that HAL seems to expose 2 devices for every USB device - 'usb' and 'usb_device', which we're mapping into just a 'usb' capability. Unless there's compelling reason to have both we might consider just filtering out one of the two.
Yeah, I meant to bring this up. HAL uses "usb_device" for the USB devices, and "usb" for the USB interface(s) provided by the device. The "usb_device" is the parent of the "usb" interface(s). The HAL "usb" namespace mirrors the attributes of the parent "usb_device", and adds a few of its own, specific to the interface. Right now I'm reporting both mostly so the device hierarchy (as defined by the parent relation) is consistent. I see two choices: (a) Just report the interface ("usb") objects and fixup their parent to be the parent of their owning USB device. Each interface object (as in HAL) will reflect the details of the parent USB device, as well as the interface-specific bits. (b) Split libvirt's "usb" capability into "usb_device" and "usb_interface". A usb_interface will always have a usb_device as it's parent (and I would *not* replicate the usb_device details in the usb_interface - that's the whole reason for keeping it as a separate device). I'm leaning fairly strongly towards (b). What do you all think?
* DeviceKit implementation is barely a proof of concept
Noticed one problem - on my build it refuses to enumerate devices if you pass in a NULL for subsystem name list. I made a really quick & dirty hack to just try it out
@@ -300,13 +301,18 @@ static int devkitNodeDriverStartup(void) }
/* Populate with known devices */ - devs = devkit_client_enumerate_by_subsystem(devkit_client, NULL, &err); - if (err) { - DEBUG0("devkit_client_enumerate_by_subsystem failed"); - devs = NULL; - goto failure; + for (i = 0 ; i < ARRAY_CARDINALITY(caps_tbl) ; i++) { + const char *caps[] = { caps_tbl[i].cap_name, NULL }; + devs = devkit_client_enumerate_by_subsystem(devkit_client, + caps, + &err); + if (err) { + DEBUG0("devkit_client_enumerate_by_subsystem failed"); + devs = NULL; + goto failure; + } + g_list_foreach(devs, dev_create, devkit_client);
Oh yeah. I forgot I fixed devkit_client_enumerate_by_subsystem to work as advertised with a NULL subsystem. I submitted the fix a couple months ago to devkit-devel@lists.freedesktop.org, but never got any acknowledgment, and haven't seen any activity on the list whatsoever, so I'm not surprised it hasn't made it in. Any idea where I should submit devkit fixes? In any case, I'll put your workaround in for now. It won't pick up devices in subsystems not listed in caps_tbl, but those are fairly useless devices anyway (since we won't recognize any device caps if we don't explicitly handle that subsystem in caps_tbl).
} - g_list_foreach(devs, dev_create, devkit_client);
driverState->privateData = devkit_client;
* need to resolve naming & other issues (see https://www.redhat.com/archives/libvir-list/2008-September/msg00430.html * ... and then fill in impl (no hurry; devkit immature now)
I'm still wondering if it is worth us santizing the device names ourselves somehow, though it might end up being rather a large job. Will have to think some more about it.
Yeah, it's a nasty issue. In a lot of ways, the HAL names are pretty nice. For example, I really like NICs named by MAC address (so knowing that a particular NIC is "eth0" is *not* encoded in the device name, but rather in a capability). This is also much nicer than naming by (e.g.) sysfs path, which encodes too much info about where the card is plugged in (that's what parent info is for). But I don't see any sign that Devkit is exposing HALish names, though I'd imagine (if only to make HAL->Devkit transition easier) that they will need to expose a device property like "HAL_UDI". With no activity on the devkit-devel list, I'm not sure where they're going. Thanks for the comments. I'll incorporate your devkit workaround, and wait on further comment before submitting the next take. Dave
This patch (for libvirt-java) implements Java bindings for the node device enumeration functions. Dave
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
David Lively