2:18 p.m.
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
3:44 a.m.
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 :|
4:47 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
8:15 a.m.
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 :|
9:01 a.m.
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
11:17 p.m.
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
12:31 p.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
8:57 a.m.
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
10:57 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
1:02 p.m.
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(a)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 :|
10:02 a.m.
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
10:14 a.m.
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 :|
10:23 a.m.
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 :|
12:20 p.m.
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
12:41 p.m.
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 :|
1:35 p.m.
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
3:03 p.m.
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 :|
11:25 a.m.
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
12:01 p.m.
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 :|
1:29 p.m.
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
3:01 p.m.
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 :|
12:45 p.m.
New subject: [libvirt] [PATCH 0/6] host ("node") device enumeration, take two
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
12:49 p.m.
New subject: [libvirt] Re: [PATCH 1/6] host ("node") device enumeration - public API additions
This patch contains the public API additions.
8:01 a.m.
New subject: [libvirt] Re: [PATCH 1/6] host ("node") device enumeration - 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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:47 a.m.
New subject: [libvirt] Re: [PATCH 1/6] host ("node") device enumeration - public API additions
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
10:39 a.m.
New subject: [libvirt] Re: [PATCH 1/6] host ("node") device enumeration - public API additions
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
12:54 p.m.
New subject: [libvirt] Re: [PATCH 2/6] host ("node") device enumeration - internal API additions
This patch contains the internal API additions.
8:13 a.m.
New subject: [libvirt] Re: [PATCH 2/6] host ("node") device enumeration - 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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
10:32 a.m.
New subject: [libvirt] Re: [PATCH 2/6] host ("node") device enumeration - internal API additions
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
10:41 a.m.
New subject: [libvirt] Re: [PATCH 2/6] host ("node") device enumeration - internal API additions
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
12:56 p.m.
New subject: [libvirt] Re: [PATCH 0/6] host ("node") device enumeration - local node drivers
This patch contains the actual HAL- and DeviceKit-based local "node"
drivers.
2:30 p.m.
New subject: [libvirt] Re: [PATCH 4/6] host ("node") device enumeration - remote node driver
This patch contains the remote impl of the node driver.
2:33 p.m.
New subject: [libvirt] Re: [PATCH 5/6] host ("node") device enumeration - virsh support
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.
2:33 p.m.
New subject: [libvirt] Re: [PATCH 6/6] host ("node") device enumeration - python bindings
This patch implements the python bindings, rather lamely. I'll submit a
new version with a proper NodeDevice object soon ...
2:15 p.m.
New subject: [libvirt] Re: [PATCH 6/6] host ("node") device enumeration - python bindings
Ok, here are proper python bindings to the node device enumeration
functions and objects.
7:53 a.m.
New subject: [libvirt] Re: [PATCH 0/6] host ("node") device enumeration, take two
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 :|
12:33 p.m.
New subject: [libvirt] Re: [PATCH 0/6] host ("node") device enumeration, take two
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(a)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
2:14 p.m.
New subject: [libvirt] Re: [PATCH 7/6] host ("node") device enumeration - Java bindings
This patch (for libvirt-java) implements Java bindings for the node
device enumeration functions.
Dave
5682
days inactive
5750
days old
37 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
David Lively