Re: [libvirt] [PATCH] add the check whether the domain has alreadyused a disk which attach
attach-disk does not give an error, when the domain which is attached has already used the source which attach. This has danger of the data corruption. I make the patch to add the check of this. This patch outputs an error, when the domain which is attached has already used the source which attach. What hypervisor were you testing with ? AFAIR, XenD should already raise an appropriate error in this scenario. XenD Don't check this when I confirm it in xen(cs 18756:5fd51e1e9c79). Therefore, I think that it had better add check to virsh.c, because this should be checked whether xen or qemu. No, because then the check is only performed for people using virsh, and would have to be duplicated across every single other application using libvirt. If it is done in the XenD or libvirtd at time of use, then all users of libvirt are automatically protected. Of course, it is the best if it guarded in Xend. But, the user may be not protected depending on cs of xen, Had better it attach check-method in libvirt.c to protect all users of libvirt ? yes the patch need to be done in libvirt.c to protect applications which are not virsh, Of course, it is the best if it guarded in Xend. But, the user may be not protected depending on cs of xen, Had better it attach check-method in libvirt.c to protect all users of libvirt ? yes the patch need to be done in libvirt.c to protect applications which are not virsh, I remake a patch so that user is protected in libvirt.c Thanks, Shigeki Sakamoto. attach-disk.patch Description: Binary data -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 2/12: Make use of versioned linker scripts
On Thu, Nov 13, 2008 at 09:44:42PM +, Daniel P. Berrange wrote: On Thu, Nov 13, 2008 at 06:31:04PM +0100, Daniel Veillard wrote: When I removed the __ from the 'private' symbols exported, this internal driver typedef now clashes with the following public symbol: okay, just one more thing, as I'm commiting the QEmu/KVM migration patch the __ cleanup will be needed for __virDomainMigratePrepare2 and __virDomainMigrateFinish2 which are introduced there, I guess it makes sense to clean them as this patch is applied too, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]: Implement Qemu/KVM live migration
On Wed, Nov 12, 2008 at 09:29:12AM +0100, Daniel Veillard wrote: On Tue, Nov 11, 2008 at 10:11:40PM +, Daniel P. Berrange wrote: Now that the QEMU bug is fixed, I'm inclined to say we should commit this migration support - minus the sleep(5) of course. It is well overdue to have migration in the QEMU driver, and oVirt guys have sanity checked its implementation in their developemnt, so shouldn't be any surprises. Agreed, +1 Done, I commited the patch, I removed the superfluous timeout and cleared a couple of things raised by syntax-check, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 4/12: Refactoring URI probing
On Thu, Nov 13, 2008 at 05:25:53PM +, Daniel P. Berrange wrote:
This patch changes the way hypervisor URIs are probed when NULL is passed
to the virConnectOpen() method. Previously we had an explicit probe method
called in each driver. This does not work when we move some drivers out of
libvirt.so and into libvirtd daemon. So I've refactored the way this works
so that we simply pass a NULL uri into the individual driver open methods.
If they see a NULL uri, they will make an attempt to open, and return a
VIR_DRV_OPEN_DECLINED code if its not suitable. This also works for the
remote driver, probing supported URI inside the daemon. Quite alot of code
churn but the end result should be functionally the same
Okay, makes sense,
[...]
+++ b/src/datatypes.c Wed Nov 12 21:59:20 2008 +
@@ -174,7 +174,7 @@
*/
static void
virReleaseConnect(virConnectPtr conn) {
-DEBUG(release connection %p %s, conn, conn-name);
+DEBUG(release connection %p, conn);
maybe conn-name should have been kept to help with debug,
each time conn-uri was set it should be easy to keep name too.
not a blocker, just a suggestion...
if (conn-domains != NULL)
virHashFree(conn-domains, (virHashDeallocator) virDomainFreeName);
if (conn-networks != NULL)
@@ -188,7 +188,7 @@
if (virLastErr.conn == conn)
virLastErr.conn = NULL;
-VIR_FREE(conn-name);
+xmlFreeURI(conn-uri);
pthread_mutex_unlock(conn-lock);
pthread_mutex_destroy(conn-lock);
@@ -213,7 +213,7 @@
return(-1);
}
pthread_mutex_lock(conn-lock);
-DEBUG(unref connection %p %s %d, conn, conn-name, conn-refs);
+DEBUG(unref connection %p %d, conn, conn-refs);
conn-refs--;
refs = conn-refs;
if (refs == 0) {
@@ -322,7 +322,7 @@
VIR_FREE(domain-name);
VIR_FREE(domain);
-DEBUG(unref connection %p %s %d, conn, conn-name, conn-refs);
+DEBUG(unref connection %p %d, conn, conn-refs);
conn-refs--;
if (conn-refs == 0) {
virReleaseConnect(conn);
@@ -458,7 +458,7 @@
VIR_FREE(network-name);
VIR_FREE(network);
-DEBUG(unref connection %p %s %d, conn, conn-name, conn-refs);
+DEBUG(unref connection %p %d, conn, conn-refs);
conn-refs--;
if (conn-refs == 0) {
virReleaseConnect(conn);
@@ -591,7 +591,7 @@
VIR_FREE(pool-name);
VIR_FREE(pool);
-DEBUG(unref connection %p %s %d, conn, conn-name, conn-refs);
+DEBUG(unref connection %p %d, conn, conn-refs);
conn-refs--;
if (conn-refs == 0) {
virReleaseConnect(conn);
@@ -729,7 +729,7 @@
VIR_FREE(vol-pool);
VIR_FREE(vol);
-DEBUG(unref connection %p %s %d, conn, conn-name, conn-refs);
+DEBUG(unref connection %p %d, conn, conn-refs);
conn-refs--;
if (conn-refs == 0) {
virReleaseConnect(conn);
diff -r d97fa69e255b src/datatypes.h
--- a/src/datatypes.h Wed Nov 12 21:05:13 2008 +
+++ b/src/datatypes.h Wed Nov 12 21:59:20 2008 +
@@ -87,7 +87,7 @@
struct _virConnect {
unsigned int magic; /* specific value to check */
int flags; /* a set of connection flags */
-char *name; /* connection URI */
+xmlURIPtr uri; /* connection URI */
/* The underlying hypervisor driver and network driver. */
virDriverPtr driver;
diff -r d97fa69e255b src/driver.h
--- a/src/driver.hWed Nov 12 21:05:13 2008 +
+++ b/src/driver.hWed Nov 12 21:59:20 2008 +
@@ -63,11 +63,8 @@
#define VIR_DRV_SUPPORTS_FEATURE(drv,conn,feature) \
((drv)-supports_feature ? (drv)-supports_feature((conn),(feature)) : 0)
-typedef const char *
-(*virDrvProbe) (void);
typedef virDrvOpenStatus
(*virDrvOpen)(virConnectPtr conn,
- xmlURIPtr uri,
virConnectAuthPtr auth,
int flags);
typedef int
@@ -302,7 +299,6 @@
int no; /* the number virDrvNo */
const char * name; /* the name of the driver */
unsigned long ver; /* the version of the backend */
-virDrvProbe probe;
virDrvOpen open;
virDrvClose close;
virDrvDrvSupportsFeature supports_feature;
diff -r d97fa69e255b src/libvirt.c
--- a/src/libvirt.c Wed Nov 12 21:05:13 2008 +
+++ b/src/libvirt.c Wed Nov 12 21:59:20 2008 +
@@ -685,8 +685,11 @@
int flags)
{
int i, res;
-virConnectPtr ret = NULL;
-xmlURIPtr uri;
+virConnectPtr ret;
+
+ret = virGetConnect();
+if (ret == NULL)
+return NULL;
/*
* If no URI is passed, then check for an environment string if not
@@ -699,74 +702,43 @@
DEBUG(Using LIBVIRT_DEFAULT_URI %s,
Re: [libvirt] PATCH: 5/12: Module build linking
On Thu, Nov 13, 2008 at 05:27:48PM +, Daniel P. Berrange wrote: This patch changes the module biuld so that stateful drivers like QEMU and LXC are directly linked into the libvirtd daemon, and not part of the libvirt.so file. It also does this for network and storage drivers. We need to export a few more symbols for this to work, and libvirtd has to explicitly initialize these drivers. Thanks to the previous patch changing the probe method, automatic driver probing still works. Okay, the whole point of the exercise ! +1 at some point libvirt package might be splitted between a libvirt and libvirt-daemon to reflect the dichotomy, but there is no hurry ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 2/12: Make use of versioned linker scripts
On Thu, Nov 13, 2008 at 05:21:23PM +, Daniel P. Berrange wrote:
This patch changes the way we use linker scripts to have fully versioned
symbols, and a versioned private section for symbols needed by libvirtd
and virsh.
If you look at the version script below, you'll see that I've added
an explicit version section for each version in which we introduced
a symbol. Some people at Red Hat have suggested to me offlist that
this is overkil, and we could just have one section named 0.5.0
for all our existing symbols, and then just add new sections for
each subsequent release.
I kind of like the idea of representing all the releases in which
we added symbols, but functionally it doesn't matter which style
we pick for our existing symbols. Anyone else have an opinion ?
diff -r bbf3d0bc9d49 src/libvirt_sym.version.in
--- /dev/null Thu Jan 01 00:00:00 1970 +
+++ b/src/libvirt_sym.version.in Tue Nov 11 17:28:31 2008 +
@@ -0,0 +1,318 @@
+/*
+ * WARNING: libvirt_sym.version.in is the master file
+ *
+ * WARNING: libvirt_sym.version is auto-generated by configure
+ */
+
+/*
+ * First officially exported symbols, for which header
+ * file definitions are installed in /usr/include/libvirt
+ * either from libvirt.h and virterror.h
+ *
+ * Versions here are *fixed* to match the libvirt version
+ * at which the symbol was introduced. This ensures that
+ * a new client app requiring symbol foo() can't accidentally
+ * run with old libvirt.so not providing foo() - the global
+ * soname version info can't enforce this since we never
+ * change the soname
+ */
+LIBVIRT_0.0.3 {
+global:
+ virConnectClose;
+ virConnectGetType;
+ virConnectGetVersion;
+ virConnectListDomains;
+ virConnectNumOfDomains;
+ virConnectOpen;
+ virConnectOpenReadOnly;
+
+ virDomainCreateLinux;
+ virDomainDestroy;
+ virDomainFree;
+ virDomainGetID;
+ virDomainGetInfo;
+ virDomainGetMaxMemory;
+ virDomainGetName;
+ virDomainGetOSType;
+ virDomainGetXMLDesc;
+ virDomainLookupByID;
+ virDomainLookupByName;
+ virDomainRestore;
+ virDomainResume;
+ virDomainSave;
+ virDomainSetMaxMemory;
+ virDomainShutdown;
+ virDomainSuspend;
+
+ virGetVersion;
+};
+
+LIBVIRT_0.0.5 {
+global:
+ virDomainLookupByUUID;
+ virDomainGetUUID;
+} LIBVIRT_0.0.3;
+
+LIBVIRT_0.1.0 {
+global:
+ virInitialize;
+ virNodeGetInfo;
+ virDomainReboot;
+
+ virCopyLastError;
+ virConnSetErrorFunc;
+ virResetLastError;
+ virErrorFunc;
+ virResetError;
+ virConnGetLastError;
+ virGetLastError;
+ virSetErrorFunc;
+ virConnCopyLastError;
+ virConnResetLastError;
+ virDefaultErrorFunc;
+} LIBVIRT_0.0.5;
+
+LIBVIRT_0.1.1 {
+global:
+ virDomainLookupByUUIDString;
+ virDomainGetUUIDString;
+ virDomainSetMemory;
+ virDomainDefineXML;
+ virDomainCreate;
+ virDomainUndefine;
+ virConnectListDefinedDomains;
+} LIBVIRT_0.1.0;
+
+LIBVIRT_0.1.4 {
+global:
+ virDomainSetVcpus;
+ virDomainPinVcpu;
+ virDomainGetVcpus;
+} LIBVIRT_0.1.1;
+
+LIBVIRT_0.1.5 {
+global:
+ virConnectNumOfDefinedDomains;
+} LIBVIRT_0.1.4;
+
+LIBVIRT_0.1.9 {
+global:
+ virDomainCoreDump;
+ virDomainAttachDevice;
+ virDomainDetachDevice;
+} LIBVIRT_0.1.5;
+
+LIBVIRT_0.2.0 {
+global:
+ virConnectNumOfNetworks;
+ virConnectListNetworks;
+ virConnectNumOfDefinedNetworks;
+ virConnectListDefinedNetworks;
+ virNetworkLookupByName;
+ virNetworkLookupByUUID;
+ virNetworkLookupByUUIDString;
+ virNetworkCreateXML;
+ virNetworkDefineXML;
+ virNetworkUndefine;
+ virNetworkCreate;
+ virNetworkDestroy;
+ virNetworkFree;
+ virNetworkGetName;
+ virNetworkGetUUID;
+ virNetworkGetUUIDString;
+ virNetworkGetXMLDesc;
+ virNetworkGetBridgeName;
+} LIBVIRT_0.1.9;
+
+LIBVIRT_0.2.1 {
+global:
+ virConnectGetCapabilities;
+ virConnectGetMaxVcpus;
+ virDomainGetMaxVcpus;
+ virDomainGetAutostart;
+ virDomainSetAutostart;
+ virNetworkGetAutostart;
+ virNetworkSetAutostart;
+} LIBVIRT_0.2.0;
+
+LIBVIRT_0.2.3 {
+global:
+ virDomainGetSchedulerType;
+ virDomainGetSchedulerParameters;
+ virDomainSetSchedulerParameters;
+} LIBVIRT_0.2.1;
+
+LIBVIRT_0.3.0 {
+global:
+ virConnectGetHostname;
+ virConnectGetURI;
+ virDomainGetConnect;
+ virNetworkGetConnect;
+} LIBVIRT_0.2.3;
+
+LIBVIRT_0.3.2 {
+global:
+ virDomainMigrate;
+ virDomainBlockStats;
+ virDomainInterfaceStats;
+} LIBVIRT_0.3.0;
+
+LIBVIRT_0.3.3 {
+global:
+ virNodeGetCellsFreeMemory;
+ virNodeGetFreeMemory;
+} LIBVIRT_0.3.2;
+
+LIBVIRT_0.4.0 {
+global:
+ virConnectOpenAuth;
+
[libvirt] RFC: Add a 'reason' argument for domain events ?
We currently have a set of domain lifecycle events VIR_DOMAIN_EVENT_ADDED = 0, VIR_DOMAIN_EVENT_REMOVED = 1, VIR_DOMAIN_EVENT_STARTED = 2, VIR_DOMAIN_EVENT_SUSPENDED = 3, VIR_DOMAIN_EVENT_RESUMED = 4, VIR_DOMAIN_EVENT_STOPPED = 5, VIR_DOMAIN_EVENT_SAVED = 6, VIR_DOMAIN_EVENT_RESTORED = 7, And a couple more I proposed last week to allow distinguishing of new domains which are transient vs persistent VIR_DOMAIN_EVENT_DEFINED = 8, VIR_DOMAIN_EVENT_UNDEFINED = 9, Previously Stefan has suggested we should consider having an event for migration, and though I rejected that at the time, I'm now inclined to agree that this info would be useful here. I'm also thinking I'd like to have more information about STOPPED STARTED events in general. eg, there are a number of reasons why an domain may have started - explicitly booted on the host - restored from a saved image - incoming migration operation and there are a number of reasons why a domain might have stopped - forcably destroyed by host admin - shutdown by host admin - shutdown by guest admin - host emulator process crashed - killed by mgmt after host emulation hung - migrated to another host - saved to a memory image We have explicit events for the SAVED/RESTORED reasons, but what should we do about the other reasons ? One option is to add alot more events VIR_DOMAIN_MIGRATED_IN (migrated to another node) VIR_DOMAIN_MIGRATED_OUT (migrated from another node) VIR_DOMAIN_SHUTDOWN (graceful shutdown by host admin) VIR_DOMAIN_DESTROYED (force destroyed by host admin) VIR_DOMAIN_CRASHED (guest kernel crashed) VIR_DOMAIN_HUNG (host emulator hung) leaving STOPPED to just be a generic stop event, with no particular reason. The downside with this, is if an application just wants to know about whether a domain shutdown, not why, then they have to track lots and lots of events. Also, not every driver would be able to provide all of these events, so would often have to fallback on a generic STOPPED event So one alternative is to provide a generic 'char * reason' with each event with provides scope on the cause of the lifecycle operation. So you'd get VIR_DOMAIN_STOPPED (crashed, shutdown, destroyed, quit, hung, migrated, saved) VIR_DOMAIN_STARTED (booted, migrated, restored) nb, we could remove the explicit SAVED and RESTORED events in this style. With such a 'reason' arg, we could possibly avoid adding the DEFINED and UNDEFINED events I suggested. Instead, adding a reason for ADDED and REMOVED events, VIR_DOMAIN_ADDED(started, defined) VIR_DOMAIN_REMOVED (shutdown, undefined) which lets you distinguish transient from persistent domains. The downside of this though, is that we can't explicitly track when a configuration file is 're-defined', eg adding a config file for an existing running guest. 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
On Wed, Nov 05, 2008 at 05:32:36PM -0500, David Lively wrote: Ugh. I see what you mean. It seems more complicated than that, though. For one thing, the AvahiWatch/AvahiPoll stuff is sufficiently abstract that it doesn't mention DBusConnection (or any other DBus type, AFAIK), so there's no indication that *any* DBusConnection is being used (though I trust that there is). Turning on AVAHI_DEBUG, I see both my (node_device_hal.c) callbacks and the avahi callbacks being called (but at different times, and with different fds and event sets). Makes me wonder whether the avahi library is using dbus_bus_get_private() to get its own DBusConnection that it doesn't have to worry about sharing with others. I tried changing the node_device_hal code to use a private dbus connection, and see exactly the same behavior (i.e., dbus immediately registers the same fd twice, with different event sets and enable state). Regardless, I like the idea of simply using this private connection (as you suggested but rejected), just for the sake of simplicity. (Also, what if Avahi's been configured out?) But ... back to the original problem. Immediately after calling dbus_set_watch_functions (and *before* initializing the Avahi stuff), I see two callbacks to watch_add with the same fd. Looking at the glib watch functions, I see watches on GIOChannels are added with g_io_add_watch(), which indeed doesn't specify what happens if the same GIOChannel is added twice. But GIOChannels aren't fds. Presumably, one could create two different GIOChannels with the same fd (via g_io_channel_unix_new) and then register watches on each of them. So I remain convinced that we need to support the registration of the same fd multiple times ... Did you ever get to the bottom of this problem. If not, then I think the only safe thing todo for this release is to change the signature of AddHandle as you suggest, so we explicitly track FDs using an integer ID that's independant of the FD number. 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 4/12: Refactoring URI probing
On Fri, Nov 14, 2008 at 11:26:49AM +0100, Daniel Veillard wrote:
On Thu, Nov 13, 2008 at 05:25:53PM +, Daniel P. Berrange wrote:
This patch changes the way hypervisor URIs are probed when NULL is passed
to the virConnectOpen() method. Previously we had an explicit probe method
called in each driver. This does not work when we move some drivers out of
libvirt.so and into libvirtd daemon. So I've refactored the way this works
so that we simply pass a NULL uri into the individual driver open methods.
If they see a NULL uri, they will make an attempt to open, and return a
VIR_DRV_OPEN_DECLINED code if its not suitable. This also works for the
remote driver, probing supported URI inside the daemon. Quite alot of code
churn but the end result should be functionally the same
Okay, makes sense,
[...]
+++ b/src/datatypes.c Wed Nov 12 21:59:20 2008 +
@@ -174,7 +174,7 @@
*/
static void
virReleaseConnect(virConnectPtr conn) {
-DEBUG(release connection %p %s, conn, conn-name);
+DEBUG(release connection %p, conn);
maybe conn-name should have been kept to help with debug,
each time conn-uri was set it should be easy to keep name too.
not a blocker, just a suggestion...
The reason I remove it, was because once we store the xmlUriPtr there
was not actually any functional code which used the 'name' field. All
the drivers' open methods will now potentially have to set 'uri', so
I felt it easier to just remove 'name', and not have possibility of one
driver forgetting to also change 'name' when changing the 'uri'.
+if (name) {
+/* Convert xen - xen:/// for back compat */
+if (STRCASEEQ(name, xen))
+name = xen:///;
+
+/* Convert xen:// - xen:/// because xmlParseURI cannot parse the
+ * former. This allows URIs such as xen://localhost to work.
+ */
+if (STREQ (name, xen://))
+name = xen:///;
+
+ret-uri = xmlParseURI (name);
+if (!ret-uri) {
+virLibConnError (ret, VIR_ERR_INVALID_ARG,
+ _(could not parse connection URI));
+goto failed;
+}
+
+DEBUG(name \%s\ to URI components:\n
+scheme %s\n
+opaque %s\n
+authority %s\n
+server %s\n
+user %s\n
+port %d\n
+path %s\n,
+ name,
+ ret-uri-scheme, ret-uri-opaque,
+ ret-uri-authority, ret-uri-server,
+ ret-uri-user, ret-uri-port,
+ ret-uri-path);
Hum... that could crash on some OSes, many of those strings might get
NULL, actually opaque will be NULL if you have path.
Hmm, doesn't printf just turn NULL into the string '(null)' ? We kind
of rely on that not crashing, more or less everywhere in libvirt.c
for the DEBUG macros. Some of these are false positives, but the vast
majority have potential for the %s argument to be NULL, since they're
accepting input directly from the public API.
$ grep DEBUG src/libvirt.c | grep '%s'
DEBUG (registering %s as driver %d,
DEBUG(libVir=%p, type=%s, typeVer=%p, libVer, type, typeVer);
DEBUG(Using LIBVIRT_DEFAULT_URI %s, defname);
DEBUG(Probed %s, latest);
DEBUG(Could not probe any hypervisor defaulting to %s,
DEBUG(Using %s as default URI, %d hypervisor found,
DEBUG(name \%s\ to URI components:\n
DEBUG(trying driver %d (%s) ...,
DEBUG(driver %d %s returned %s,
DEBUG(network driver %d %s returned %s,
DEBUG(storage driver %d %s returned %s,
DEBUG(name=%s, name);
DEBUG(name=%s, name);
DEBUG(name=%s, auth=%p, flags=%d, name, auth, flags);
DEBUG(conn=%p, type=%s, conn, type);
DEBUG(conn=%p, xmlDesc=%s, flags=%d, conn, xmlDesc, flags);
DEBUG(conn=%p, uuid=%s, conn, uuid);
DEBUG(conn=%p, uuidstr=%s, conn, uuidstr);
DEBUG(conn=%p, name=%s, conn, name);
DEBUG(domain=%p, to=%s, domain, to);
DEBUG(conn=%p, from=%s, conn, from);
DEBUG(domain=%p, to=%s, flags=%d, domain, to, flags);
DEBUG(domain=%p, dconn=%p, flags=%lu, dname=%s, uri=%s, bandwidth=%lu,
DEBUG(dconn=%p, cookie=%p, cookielen=%p, uri_in=%s, uri_out=%p, flags=%lu,
dname=%s, bandwidth=%lu, dconn, cookie, cookielen, uri_in, uri_out, flags,
dname, bandwidth);
DEBUG(domain=%p, cookie=%p, cookielen=%d, uri=%s, flags=%lu, dname=%s,
bandwidth=%lu, domain, cookie, cookielen, uri, flags, dname, bandwidth);
DEBUG(dconn=%p, dname=%s, cookie=%p, cookielen=%d, uri=%s, flags=%lu,
dconn, dname, cookie, cookielen, uri, flags);
DEBUG(domain=%p, path=%s, stats=%p, size=%zi, dom, path, stats, size);
DEBUG(domain=%p, path=%s, stats=%p, size=%zi, dom, path, stats, size);
DEBUG(domain=%p, path=%s, offset=%lld, size=%zi, buffer=%p,
DEBUG(conn=%p, xml=%s, conn, xml);
DEBUG(domain=%p, xml=%s, domain, xml);
DEBUG(domain=%p,
Re: [libvirt] PATCH: 6/12: Optional dlopen() support
On Thu, Nov 13, 2008 at 05:29:15PM +, Daniel P. Berrange wrote:
This patch is a small incremental change to optionally allow us to build
every driver as a dlopen()able module. This is disabled by default for
now. I'm not sure whether this is hugely usefl or not, but it was easy
enough to do, so here's the patch.
Good, I note there is also some renaming of entry points...
I'm just a bit surprized it affects tests too ...
+++ b/tests/xmconfigtest.cThu Nov 13 17:13:02 2008 +
[...]
virDomainDefFree(def);
-if (conn) {
-conn-privateData = old_priv;
-virConnectClose(conn);
-}
+VIR_FREE(conn);
[...]
-if (conn) {
-conn-privateData = old_priv;
-virConnectClose(conn);
-}
+VIR_FREE(conn);
hum, that's a bit surprizing, we are not closing the connection
to free the data structure anymore ... I assume there is a good reason
but that's obscure.
+1
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
[EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 7/12: Public API for node devices
On Thu, Nov 13, 2008 at 05:30:29PM +, Daniel P. Berrange wrote: This patch is the public API parts of the node device enumeration code. No changes since David's last submission of this, except for some Makefile tweaks okidoc, reviewed many times already, +1 Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] avoid format string warnings
From f06595fb110e1e5a4c4f6ea8c729da3341f89ac4 Mon Sep 17 00:00:00 2001
From: Jim Meyering [EMAIL PROTECTED]
Date: Fri, 14 Nov 2008 13:19:29 +0100
Subject: [PATCH] avoid format string warnings
* src/openvz_driver.c (ADD_ARG_LIT): Add %s arg before _(...).
* src/qemu_driver.c (PCI_ATTACH_OK_MSG): Likewise.
* src/util.c (virExec, virRun): Likewise.
---
src/openvz_driver.c |4 ++--
src/qemu_driver.c |2 +-
src/util.c |4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/openvz_driver.c b/src/openvz_driver.c
index 48ffa13..95631ee 100644
--- a/src/openvz_driver.c
+++ b/src/openvz_driver.c
@@ -426,7 +426,7 @@ openvzDomainSetNetwork(virConnectPtr conn, const char
*vpsid,
dev_name_ve = openvzGenerateContainerVethName(veid);
if (dev_name_ve == NULL) {
openvzError(conn, VIR_ERR_INTERNAL_ERROR,
-_(Could not generate eth name for container));
+ %s, _(Could not generate eth name for container));
rc = -1;
goto exit;
}
@@ -437,7 +437,7 @@ openvzDomainSetNetwork(virConnectPtr conn, const char
*vpsid,
net-ifname = openvzGenerateVethName(veid, dev_name_ve);
if (net-ifname == NULL) {
openvzError(conn, VIR_ERR_INTERNAL_ERROR,
-_(Could not generate veth name));
+ %s, _(Could not generate veth name));
rc = -1;
VIR_FREE(dev_name_ve);
goto exit;
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 8291bfe..47167b7 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -2611,7 +2611,7 @@ static int qemudDomainAttachPciDiskDevice(virDomainPtr
dom, virDomainDeviceDefPt
s += strlen(PCI_ATTACH_OK_MSG);
if (virStrToLong_i ((const char*)s, dummy, 10,
dev-data.disk-slotnum) == -1)
-qemudLog(QEMUD_WARN, _(Unable to parse slot number\n));
+qemudLog(QEMUD_WARN, %s, _(Unable to parse slot number\n));
} else {
qemudReportError (dom-conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
_(adding %s disk failed), type);
diff --git a/src/util.c b/src/util.c
index 6141847..9b1c5f4 100644
--- a/src/util.c
+++ b/src/util.c
@@ -406,7 +406,7 @@ virExec(virConnectPtr conn,
char *argv_str;
if ((argv_str = virArgvToString(argv)) == NULL) {
-ReportError(conn, VIR_ERR_NO_MEMORY, _(command debug string));
+ReportError(conn, VIR_ERR_NO_MEMORY, %s, _(command debug string));
return -1;
}
DEBUG0(argv_str);
@@ -523,7 +523,7 @@ virRun(virConnectPtr conn,
char *argv_str = NULL;
if ((argv_str = virArgvToString(argv)) == NULL) {
-ReportError(conn, VIR_ERR_NO_MEMORY, _(command debug string));
+ReportError(conn, VIR_ERR_NO_MEMORY, %s, _(command debug string));
goto error;
}
DEBUG0(argv_str);
--
1.6.0.4.911.gc990
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
RE: [libvirt] RFC: Add a 'reason' argument for domain events ?
To revisit the DEFINED, UNDEFINED topic - I have been giving this some thought, and it seems like it is redundant information to me. Doesn't the existence of a config file define the difference between a transient and a persistent domain? Therefore this could be tracked solely with the enum values we have? If a transient domain becomes persistent via adding a config file, the ADDED event would be emitted, denoting a config file came into being, and by definition changing this domain from transient, to persistent. I'm not sure I understand the need for these events. Moving on to the shutdown reason discussion - I can certainly see the need to know why a domain stopped. That said - would it not be more consistent with the API to introduce an event sub-type enum to be passed alongside of the event-type, passed as a second integer? This would reduce the size of the wire protocol, not needing to pass the full character string, as well. This would trivially allow us finer grained notifications within a particular event. A downside is that it may become a bit noisy for clients who may not be interested in all of the sub-events. Perhaps we should add some sort of filtering mechanism? -Original Message- From: [EMAIL PROTECTED] on behalf of Daniel P. Berrange Sent: Fri 11/14/2008 6:18 AM To: libvir-list@redhat.com Subject: [libvirt] RFC: Add a 'reason' argument for domain events ? We currently have a set of domain lifecycle events VIR_DOMAIN_EVENT_ADDED = 0, VIR_DOMAIN_EVENT_REMOVED = 1, VIR_DOMAIN_EVENT_STARTED = 2, VIR_DOMAIN_EVENT_SUSPENDED = 3, VIR_DOMAIN_EVENT_RESUMED = 4, VIR_DOMAIN_EVENT_STOPPED = 5, VIR_DOMAIN_EVENT_SAVED = 6, VIR_DOMAIN_EVENT_RESTORED = 7, And a couple more I proposed last week to allow distinguishing of new domains which are transient vs persistent VIR_DOMAIN_EVENT_DEFINED = 8, VIR_DOMAIN_EVENT_UNDEFINED = 9, Previously Stefan has suggested we should consider having an event for migration, and though I rejected that at the time, I'm now inclined to agree that this info would be useful here. I'm also thinking I'd like to have more information about STOPPED STARTED events in general. eg, there are a number of reasons why an domain may have started - explicitly booted on the host - restored from a saved image - incoming migration operation and there are a number of reasons why a domain might have stopped - forcably destroyed by host admin - shutdown by host admin - shutdown by guest admin - host emulator process crashed - killed by mgmt after host emulation hung - migrated to another host - saved to a memory image We have explicit events for the SAVED/RESTORED reasons, but what should we do about the other reasons ? One option is to add alot more events VIR_DOMAIN_MIGRATED_IN (migrated to another node) VIR_DOMAIN_MIGRATED_OUT (migrated from another node) VIR_DOMAIN_SHUTDOWN (graceful shutdown by host admin) VIR_DOMAIN_DESTROYED (force destroyed by host admin) VIR_DOMAIN_CRASHED (guest kernel crashed) VIR_DOMAIN_HUNG (host emulator hung) leaving STOPPED to just be a generic stop event, with no particular reason. The downside with this, is if an application just wants to know about whether a domain shutdown, not why, then they have to track lots and lots of events. Also, not every driver would be able to provide all of these events, so would often have to fallback on a generic STOPPED event So one alternative is to provide a generic 'char * reason' with each event with provides scope on the cause of the lifecycle operation. So you'd get VIR_DOMAIN_STOPPED (crashed, shutdown, destroyed, quit, hung, migrated, saved) VIR_DOMAIN_STARTED (booted, migrated, restored) nb, we could remove the explicit SAVED and RESTORED events in this style. With such a 'reason' arg, we could possibly avoid adding the DEFINED and UNDEFINED events I suggested. Instead, adding a reason for ADDED and REMOVED events, VIR_DOMAIN_ADDED(started, defined) VIR_DOMAIN_REMOVED (shutdown, undefined) which lets you distinguish transient from persistent domains. The downside of this though, is that we can't explicitly track when a configuration file is 're-defined', eg adding a config file for an existing running guest. 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: Add a 'reason' argument for domain events ?
On Fri, Nov 14, 2008 at 11:18:48AM +, Daniel P. Berrange wrote: [...] One option is to add alot more events VIR_DOMAIN_MIGRATED_IN (migrated to another node) VIR_DOMAIN_MIGRATED_OUT (migrated from another node) VIR_DOMAIN_SHUTDOWN (graceful shutdown by host admin) VIR_DOMAIN_DESTROYED (force destroyed by host admin) VIR_DOMAIN_CRASHED (guest kernel crashed) VIR_DOMAIN_HUNG (host emulator hung) leaving STOPPED to just be a generic stop event, with no particular reason. The downside with this, is if an application just wants to know about whether a domain shutdown, not why, then they have to track lots and lots of events. Also, not every driver would be able to provide all of these events, so would often have to fallback on a generic STOPPED event yes that makes using the API way harder than it should, though enumerating the cases: in a switch is not that hard... So one alternative is to provide a generic 'char * reason' with each event with provides scope on the cause of the lifecycle operation. So you'd get VIR_DOMAIN_STOPPED (crashed, shutdown, destroyed, quit, hung, migrated, saved) VIR_DOMAIN_STARTED (booted, migrated, restored) nb, we could remove the explicit SAVED and RESTORED events in this style. With such a 'reason' arg, we could possibly avoid adding the DEFINED and UNDEFINED events I suggested. Instead, adding a reason for ADDED and REMOVED events, VIR_DOMAIN_ADDED(started, defined) VIR_DOMAIN_REMOVED (shutdown, undefined) which lets you distinguish transient from persistent domains. The downside of this though, is that we can't explicitly track when a configuration file is 're-defined', eg adding a config file for an existing running guest. I like this, I wonder if a free form part to the message might make sense, even if we don't have currently a way to pass this say from virsh command line. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
RE: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
I'll answer for Dave, while I'm looking at this. As far as I know, Dave is of the opinion that we are just getting lucky using the APIs as we are, and remains convinced that his suggested change is necessary here. He (and I) remain worried that release of the EventImpl API without this API change could get us into trouble in the future, as we would have to support the released API that has different semantics than DBus, which we were supposed to be modeled closely to. You had sounded convinced it was not necessary the last we heard though...and ultimatley we don't have checkin permissions...so we'll go with whatever you guys decide. I'll let Dave follow up with any additional examples of where this would get us in trouble... -Original Message- From: Daniel P. Berrange [mailto:[EMAIL PROTECTED] Sent: Fri 11/14/2008 6:27 AM To: Dave Lively Cc: Ben Guthro; libvir-list Subject: Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent On Wed, Nov 05, 2008 at 05:32:36PM -0500, David Lively wrote: Ugh. I see what you mean. It seems more complicated than that, though. For one thing, the AvahiWatch/AvahiPoll stuff is sufficiently abstract that it doesn't mention DBusConnection (or any other DBus type, AFAIK), so there's no indication that *any* DBusConnection is being used (though I trust that there is). Turning on AVAHI_DEBUG, I see both my (node_device_hal.c) callbacks and the avahi callbacks being called (but at different times, and with different fds and event sets). Makes me wonder whether the avahi library is using dbus_bus_get_private() to get its own DBusConnection that it doesn't have to worry about sharing with others. I tried changing the node_device_hal code to use a private dbus connection, and see exactly the same behavior (i.e., dbus immediately registers the same fd twice, with different event sets and enable state). Regardless, I like the idea of simply using this private connection (as you suggested but rejected), just for the sake of simplicity. (Also, what if Avahi's been configured out?) But ... back to the original problem. Immediately after calling dbus_set_watch_functions (and *before* initializing the Avahi stuff), I see two callbacks to watch_add with the same fd. Looking at the glib watch functions, I see watches on GIOChannels are added with g_io_add_watch(), which indeed doesn't specify what happens if the same GIOChannel is added twice. But GIOChannels aren't fds. Presumably, one could create two different GIOChannels with the same fd (via g_io_channel_unix_new) and then register watches on each of them. So I remain convinced that we need to support the registration of the same fd multiple times ... Did you ever get to the bottom of this problem. If not, then I think the only safe thing todo for this release is to change the signature of AddHandle as you suggest, so we explicitly track FDs using an integer ID that's independant of the FD number. 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] avoid format string warnings
On Fri, Nov 14, 2008 at 01:22:26PM +0100, Jim Meyering wrote: From f06595fb110e1e5a4c4f6ea8c729da3341f89ac4 Mon Sep 17 00:00:00 2001 From: Jim Meyering [EMAIL PROTECTED] Date: Fri, 14 Nov 2008 13:19:29 +0100 Subject: [PATCH] avoid format string warnings * src/openvz_driver.c (ADD_ARG_LIT): Add %s arg before _(...). * src/qemu_driver.c (PCI_ATTACH_OK_MSG): Likewise. * src/util.c (virExec, virRun): Likewise. Oops, sure, +1 ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 2/12: Make use of versioned linker scripts
On Fri, Nov 14, 2008 at 10:37:51AM +, Daniel P. Berrange wrote: On Thu, Nov 13, 2008 at 05:21:23PM +, Daniel P. Berrange wrote: This patch changes the way we use linker scripts to have fully versioned symbols, and a versioned private section for symbols needed by libvirtd and virsh. If you look at the version script below, you'll see that I've added an explicit version section for each version in which we introduced a symbol. Some people at Red Hat have suggested to me offlist that this is overkil, and we could just have one section named 0.5.0 for all our existing symbols, and then just add new sections for each subsequent release. I kind of like the idea of representing all the releases in which we added symbols, but functionally it doesn't matter which style we pick for our existing symbols. Anyone else have an opinion ? keeping the log may also help maintaining http://libvirt.org/hvsupport.html so I'm in favor of the full list Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 4/12: Refactoring URI probing
On Fri, Nov 14, 2008 at 11:35:16AM +, Daniel P. Berrange wrote:
On Fri, Nov 14, 2008 at 11:26:49AM +0100, Daniel Veillard wrote:
On Thu, Nov 13, 2008 at 05:25:53PM +, Daniel P. Berrange wrote:
This patch changes the way hypervisor URIs are probed when NULL is passed
to the virConnectOpen() method. Previously we had an explicit probe method
called in each driver. This does not work when we move some drivers out of
libvirt.so and into libvirtd daemon. So I've refactored the way this works
so that we simply pass a NULL uri into the individual driver open methods.
If they see a NULL uri, they will make an attempt to open, and return a
VIR_DRV_OPEN_DECLINED code if its not suitable. This also works for the
remote driver, probing supported URI inside the daemon. Quite alot of code
churn but the end result should be functionally the same
Okay, makes sense,
[...]
+++ b/src/datatypes.c Wed Nov 12 21:59:20 2008 +
@@ -174,7 +174,7 @@
*/
static void
virReleaseConnect(virConnectPtr conn) {
-DEBUG(release connection %p %s, conn, conn-name);
+DEBUG(release connection %p, conn);
maybe conn-name should have been kept to help with debug,
each time conn-uri was set it should be easy to keep name too.
not a blocker, just a suggestion...
The reason I remove it, was because once we store the xmlUriPtr there
was not actually any functional code which used the 'name' field. All
the drivers' open methods will now potentially have to set 'uri', so
I felt it easier to just remove 'name', and not have possibility of one
driver forgetting to also change 'name' when changing the 'uri'.
okay
+if (name) {
+/* Convert xen - xen:/// for back compat */
+if (STRCASEEQ(name, xen))
+name = xen:///;
+
+/* Convert xen:// - xen:/// because xmlParseURI cannot parse the
+ * former. This allows URIs such as xen://localhost to work.
+ */
+if (STREQ (name, xen://))
+name = xen:///;
+
+ret-uri = xmlParseURI (name);
+if (!ret-uri) {
+virLibConnError (ret, VIR_ERR_INVALID_ARG,
+ _(could not parse connection URI));
+goto failed;
+}
+
+DEBUG(name \%s\ to URI components:\n
+scheme %s\n
+opaque %s\n
+authority %s\n
+server %s\n
+user %s\n
+port %d\n
+path %s\n,
+ name,
+ ret-uri-scheme, ret-uri-opaque,
+ ret-uri-authority, ret-uri-server,
+ ret-uri-user, ret-uri-port,
+ ret-uri-path);
Hum... that could crash on some OSes, many of those strings might get
NULL, actually opaque will be NULL if you have path.
Hmm, doesn't printf just turn NULL into the string '(null)' ? We kind
Linux one yes, others no :-)
of rely on that not crashing, more or less everywhere in libvirt.c
for the DEBUG macros. Some of these are false positives, but the vast
majority have potential for the %s argument to be NULL, since they're
accepting input directly from the public API.
$ grep DEBUG src/libvirt.c | grep '%s'
DEBUG (registering %s as driver %d,
DEBUG(libVir=%p, type=%s, typeVer=%p, libVer, type, typeVer);
DEBUG(Using LIBVIRT_DEFAULT_URI %s, defname);
DEBUG(Probed %s, latest);
DEBUG(Could not probe any hypervisor defaulting to %s,
DEBUG(Using %s as default URI, %d hypervisor found,
DEBUG(name \%s\ to URI components:\n
DEBUG(trying driver %d (%s) ...,
DEBUG(driver %d %s returned %s,
DEBUG(network driver %d %s returned %s,
DEBUG(storage driver %d %s returned %s,
DEBUG(name=%s, name);
DEBUG(name=%s, name);
DEBUG(name=%s, auth=%p, flags=%d, name, auth, flags);
DEBUG(conn=%p, type=%s, conn, type);
DEBUG(conn=%p, xmlDesc=%s, flags=%d, conn, xmlDesc, flags);
DEBUG(conn=%p, uuid=%s, conn, uuid);
DEBUG(conn=%p, uuidstr=%s, conn, uuidstr);
DEBUG(conn=%p, name=%s, conn, name);
DEBUG(domain=%p, to=%s, domain, to);
DEBUG(conn=%p, from=%s, conn, from);
DEBUG(domain=%p, to=%s, flags=%d, domain, to, flags);
DEBUG(domain=%p, dconn=%p, flags=%lu, dname=%s, uri=%s, bandwidth=%lu,
DEBUG(dconn=%p, cookie=%p, cookielen=%p, uri_in=%s, uri_out=%p,
flags=%lu, dname=%s, bandwidth=%lu, dconn, cookie, cookielen, uri_in,
uri_out, flags, dname, bandwidth);
DEBUG(domain=%p, cookie=%p, cookielen=%d, uri=%s, flags=%lu, dname=%s,
bandwidth=%lu, domain, cookie, cookielen, uri, flags, dname, bandwidth);
DEBUG(dconn=%p, dname=%s, cookie=%p, cookielen=%d, uri=%s, flags=%lu,
dconn, dname, cookie, cookielen, uri, flags);
DEBUG(domain=%p, path=%s, stats=%p, size=%zi, dom, path, stats, size);
DEBUG(domain=%p, path=%s,
Re: [libvirt] PATCH: 6/12: Optional dlopen() support
On Fri, Nov 14, 2008 at 01:05:18PM +0100, Daniel Veillard wrote:
On Thu, Nov 13, 2008 at 05:29:15PM +, Daniel P. Berrange wrote:
This patch is a small incremental change to optionally allow us to build
every driver as a dlopen()able module. This is disabled by default for
now. I'm not sure whether this is hugely usefl or not, but it was easy
enough to do, so here's the patch.
Good, I note there is also some renaming of entry points...
I'm just a bit surprized it affects tests too ...
+++ b/tests/xmconfigtest.c Thu Nov 13 17:13:02 2008 +
[...]
virDomainDefFree(def);
-if (conn) {
-conn-privateData = old_priv;
-virConnectClose(conn);
-}
+VIR_FREE(conn);
[...]
-if (conn) {
-conn-privateData = old_priv;
-virConnectClose(conn);
-}
+VIR_FREE(conn);
hum, that's a bit surprizing, we are not closing the connection
to free the data structure anymore ... I assume there is a good reason
but that's obscure.
Previously we were seriously abusing the virConnectPtr struct by
getting an instance with test:///default URI, and then munging
the private Data to point to XM's struct. I stopped opening a
connection, and just have a struct directly initialized with
the test data, so there's no need to explicitly close it - its
a little clearer if you look further up the test case, rather
than just this bit of diff context.
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 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 9/12: HAL/DevitKit node device impl
On Thu, Nov 13, 2008 at 05:33:20PM +, Daniel P. Berrange wrote: This is the main implementation of the local device enumation driver. The main change since David's patch is that we hav a single nodedevRegister() API call, instead of initializing HAL/DevKit separtely. This was needed to make the dlopen() support easier to implement. Functionally the end result is the same. again, largely reviewed already, +1 Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 11/12: virsh support
On Thu, Nov 13, 2008 at 05:35:09PM +, Daniel P. Berrange wrote: This patch adds two node virsh commands for the node device enumeration APIs. The only change here is to change the command names to have a shorter prefix, nodedev-list and nodedev-dumpxml okay then +1 Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 12/12: python bindings for node devices
On Thu, Nov 13, 2008 at 05:36:06PM +, Daniel P. Berrange wrote: This is the python bindings for node devices. No change from David's last patches. sure, +1 Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 8/12:Interal driver API for node devices
On Thu, 2008-11-13 at 17:31 +, Daniel P. Berrange wrote:
This is the basic internal driver support code for the node device APIs.
The actual registration of the drivers was pushed to a later patch to
allow this to compile on its own thus be fully bisectable.
Daniel
diff -r 6812c3044043 src/Makefile.am
--- a/src/Makefile.am Wed Nov 12 21:11:46 2008 +
+++ b/src/Makefile.am Wed Nov 12 21:11:51 2008 +
@@ -132,6 +132,10 @@
storage_driver.h storage_driver.c \
storage_backend.h storage_backend.c
+# Network driver generic impl APIs
Typo
+NODE_DEVICE_CONF_SOURCES = \
+ node_device_conf.c node_device_conf.h
+
...
diff -r 6812c3044043 src/node_device_conf.c
--- /dev/null Thu Jan 01 00:00:00 1970 +
+++ b/src/node_device_conf.c Wed Nov 12 21:11:51 2008 +
...
+char *virNodeDeviceDefFormat(virConnectPtr conn,
+ const virNodeDeviceDefPtr def)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+virNodeDevCapsDefPtr caps = def-caps;
+char *tmp;
+
+virBufferAddLit(buf, device\n);
+virBufferEscapeString(buf, name%s/name\n, def-name);
+
+if (def-parent)
+virBufferEscapeString(buf, parent%s/parent\n, def-parent);
+
+for (caps = def-caps; caps; caps = caps-next) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+union _virNodeDevCapData *data = caps-data;
+
+virBufferVSprintf(buf, capability type='%s'\n,
+ virNodeDevCapTypeToString(caps-type));
+switch (caps-type) {
...
+case VIR_NODE_DEV_CAP_NET:
+virBufferVSprintf(buf, interface%s/interface\n,
+ data-net.interface);
+if (data-net.address)
+virBufferVSprintf(buf, address%s/address\n,
+ data-net.address);
+if (data-net.subtype != VIR_NODE_DEV_CAP_NET_LAST) {
+const char *subtyp =
+virNodeDevNetCapTypeToString(data-net.subtype);
+virBufferVSprintf(buf, capability type='%s'\n,
subtyp);
+switch (data-net.subtype) {
+case VIR_NODE_DEV_CAP_NET_80203:
+virBufferVSprintf(buf,
+
mac_address%012llx/address\n,
+ data-net.data.ieee80203.mac_address);
+break;
+case VIR_NODE_DEV_CAP_NET_80211:
+virBufferVSprintf(buf,
+
mac_address%012llx/address\n,
+ data-net.data.ieee80211.mac_address);
+break;
+case VIR_NODE_DEV_CAP_NET_LAST:
+/* Keep dumb compiler happy */
+break;
+}
+virBufferAddLit(buf, /capability\n);
This all seems a bit odd.
We've listed the mac address once already, so why do it again in a hex format?
And I wouldn't think of 802.3 vs 802.11 as a network device capability, but
more like it's type - they're mutually exclusive, right?
Also, we have:
device
...
capability type='net'
...
capability type='80203'
mac_address001320f74a06/address
/capability
/capability
/device
i.e. two different capability semantics?
+}
+break;
+case VIR_NODE_DEV_CAP_BLOCK:
+virBufferVSprintf(buf, device%s/device\n,
+ data-block.device);
Two nested device nodes:
device
...
capability type='block'
device/dev/sda1/device
/capability
/device
How about path or device_path ?
+break;
+case VIR_NODE_DEV_CAP_SCSI_HOST:
+virBufferVSprintf(buf, host%d/host\n,
+ data-scsi_host.host);
+break;
+case VIR_NODE_DEV_CAP_SCSI:
+virBufferVSprintf(buf, host%d/host\n,
data-scsi.host);
+virBufferVSprintf(buf, bus%d/bus\n, data-scsi.bus);
+virBufferVSprintf(buf, target%d/target\n,
+ data-scsi.target);
+virBufferVSprintf(buf, lun%d/lun\n, data-scsi.lun);
+if (data-scsi.type)
+virBufferVSprintf(buf, type%s/type\n,
+ data-scsi.type);
+break;
+case VIR_NODE_DEV_CAP_STORAGE:
+if (data-storage.bus)
+virBufferVSprintf(buf, bus%s/bus\n,
+ data-storage.bus);
+if (data-storage.drive_type)
+virBufferVSprintf(buf, drive_type%s/drive_type\n,
+ data-storage.drive_type);
+if (data-storage.originating_device)
+
Re: [libvirt] PATCH: 7/12: Public API for node devices
On Fri, Nov 14, 2008 at 12:46:09PM +, Mark McLoughlin wrote: On Thu, 2008-11-13 at 17:30 +, Daniel P. Berrange wrote: This patch is the public API parts of the node device enumeration code. No changes since David's last submission of this, except for some Makefile tweaks + +int virNodeNumOfDevices (virConnectPtr conn, + unsigned int flags); + +int virNodeListDevices (virConnectPtr conn, + char **const names, + int maxnames, + unsigned int flags); + +int virNodeNumOfDevicesByCap (virConnectPtr conn, + const char *cap, + unsigned int flags); + +int virNodeListDevicesByCap (virConnectPtr conn, + const char *cap, + char **const names, + int maxnames, + unsigned int flags); How about combining these two sets of functions and if the capability type isn't supplied list all devices? Yes, we could just remove the ByCap APIs, and add the 'const char *cap' arg to the first two APIs, allowing NULL. + +virNodeDevicePtrvirNodeDeviceLookupByName (virConnectPtr conn, + const char *name); + +const char *virNodeDeviceGetName (virNodeDevicePtr dev); + How stable are these names? e.g. should we say that no-one should rely on the format of the name and that the name of a given device could change across node reboots? Even if HAL guarantees the name to be stable (does it?), if you switch between HAL and DevKit it could change, right? I don't think HAL explicitly guarentees it, it merely happens to have been stable AFAICT. The naming is definitely completely different between HAL and DevKit. This is probably my biggest worry with the impl so far - some app using it will need to have a stable identifier for a device and we won't be providing it. We could invent our own stable naming scheme for devices - the scheme would vary per capability - eg for PCI devices we can use the bus, function, slot identifiers. USB is hard to guarentee though - if a device is plugged in unpluged plugged in again it won't get the same address, and there's no real other identifier we can rely on for this. +const char *virNodeDeviceGetParent (virNodeDevicePtr dev); A GetParent() but no ListChildren() seems a little strange ... Some of the hierarchy seems a little strange to me, e.g. # ./virsh node-list-devices --cap net ... net_00_13_20_f7_4a_06 # ./virsh node-device-dumpxml net_00_13_20_f7_4a_06 device namenet_00_13_20_f7_4a_06/name parentpci_8086_10de/parent capability type='net' interfaceeth0/interface address00:13:20:f7:4a:06/address capability type='80203' mac_address001320f74a06/address /capability /capability /device # ./virsh node-device-dumpxml pci_8086_10de device namepci_8086_10de/name parentcomputer/parent capability type='pci' domain0/domain bus0/bus slot25/slot function0/function product id='4318'82567LM-3 Gigabit Network Connection/product vendor id='32902'Intel Corporation/vendor /capability /device I see that all this naturally flows from the way hal does things, but the pci device isn't a parent of the network device in any real sense ... I guess I would expect to just see one device with both the 'net' and 'pci' capabilities. So that's basically removing all explicit tracking of 'logical' devices (NICs, disk, etc) and only ever representing 'physical' devices (PCI, USB devices). The problem I can see is that one physical device can result in many logical devices - even multiple instances of the same capability - particularly multi-function USB devices can result in a large number of sub-devices for audio, video, etc. Or a SCSI host adapter can have a single PCI device, but result in multiple host adapters in the OS view. Separating the physical from logical devices gives us the opportunity to define more stable names for devices with certain capabilities. eg, for a USB network card, its hard to invent a stable name at the level of the USB device, but for the logical NIC you can easily invent a name based off the MAC address. +int virNodeDeviceNumOfCaps (virNodeDevicePtr dev); + +int virNodeDeviceListCaps(virNodeDevicePtr dev, + char **const names, +
Re: [libvirt] PATCH: 9/12: HAL/DevitKit node device impl
On Thu, 2008-11-13 at 17:33 +, Daniel P. Berrange wrote:
This is the main implementation of the local device enumation driver. The
main change since David's patch is that we hav a single nodedevRegister()
API call, instead of initializing HAL/DevKit separtely. This was needed
to make the dlopen() support easier to implement. Functionally the end
result is the same.
The DeviceKit implementation seems to be much more of a work-in-progress
than the HAL implementation - maybe disable it by default in configure?
(i.e. with_devkit=no instead of with_devkit=check)
diff -r acac4fc31665 src/node_device.c
--- /dev/null Thu Jan 01 00:00:00 1970 +
+++ b/src/node_device.c Thu Nov 13 17:10:30 2008 +
...
+static int nodeListDevicesByCap(virConnectPtr conn,
+const char *cap,
+char **const names,
+int maxnames,
+unsigned int flags ATTRIBUTE_UNUSED)
+{
+virDeviceMonitorStatePtr driver = conn-devMonPrivateData;
+int ndevs = 0;
+unsigned int i;
+
+for (i = 0; i driver-devs.count ndevs maxnames; i++)
+if (dev_has_cap(driver-devs.objs[i], cap))
+if ((names[ndevs++] = strdup(driver-devs.objs[i]-def-name))
== NULL)
+goto failure;
Over 80 columns here and would be easier to read if split up
+
+return ndevs;
+
+ failure:
+--ndevs;
+while (--ndevs = 0)
+VIR_FREE(names[ndevs]);
+return -1;
+}
+
...
diff -r acac4fc31665 src/node_device_devkit.c
--- /dev/null Thu Jan 01 00:00:00 1970 +
+++ b/src/node_device_devkit.cThu Nov 13 17:10:30 2008 +
...
+static int gather_net_cap(DevkitDevice *dkdev,
+ union _virNodeDevCapData *d)
+{
+const char *sysfs_path = devkit_device_get_native_path(dkdev);
+const char *interface;
+
+if (sysfs_path == NULL)
+return -1;
+interface = strrchr(sysfs_path, '/');
+if (!interface *interface *(++interface))
+return -1;
Was this meant to be:
if (!interface || !*interface || !*(++interface))
+if ((d-net.interface = strdup(interface)) == NULL)
+return -1;
+
+d-net.subtype = VIR_NODE_DEV_CAP_NET_LAST;
+
+return 0;
+}
+
+
...
+static void dev_create(void *_dkdev, void *_dkclient ATTRIBUTE_UNUSED)
+{
+DevkitDevice *dkdev = _dkdev;
+const char *sysfs_path = devkit_device_get_native_path(dkdev);
+virNodeDeviceObjPtr dev = NULL;
+const char *name;
+int rv;
+
+if (sysfs_path == NULL)
+/* Currently using basename(sysfs_path) as device name (key) */
+return;
+
+name = strrchr(sysfs_path, '/');
+if (name == NULL)
+name = sysfs_path;
+else
+++name;
+
+if (VIR_ALLOC(dev) 0 || VIR_ALLOC(dev-def) 0)
+goto failure;
+
+dev-privateData = dkdev;
You need need a privateFree() hook here to do g_object_unref(), I think
+
+if ((dev-def-name = strdup(name)) == NULL)
+goto failure;
+
+// TODO: Find device parent, if any
+
+rv = gather_capabilities(dkdev, dev-def-caps);
+if (rv != 0) goto failure;
+
+if (VIR_REALLOC_N(driverState-devs.objs, driverState-devs.count + 1)
0)
+goto failure;
+
+driverState-devs.objs[driverState-devs.count++] = dev;
Add a virNodeDeviceObjAdd() function for this?
+
+return;
+
+ failure:
+DEBUG(FAILED TO ADD dev %s, name);
+if (dev)
+virNodeDeviceDefFree(dev-def);
+VIR_FREE(dev);
+}
+
+
+static int devkitDeviceMonitorStartup(void)
+{
+size_t caps_tbl_len = sizeof(caps_tbl) / sizeof(caps_tbl[0]);
+DevkitClient *devkit_client = NULL;
+GError *err = NULL;
+GList *devs;
+int i;
+
+/* Ensure caps_tbl is sorted by capability name */
+qsort(caps_tbl, caps_tbl_len, sizeof(caps_tbl[0]), cmpstringp);
+
+if (VIR_ALLOC(driverState) 0)
+return -1;
+
+// TODO: Is it really ok to call this multiple times??
+// Is there something analogous to call on close?
+g_type_init();
Yep, it's fine to call multiple times and there's no shutdown version.
+
+/* Get new devkit_client and connect to daemon */
+devkit_client = devkit_client_new(NULL);
+if (devkit_client == NULL) {
+DEBUG0(devkit_client_new returned NULL);
+goto failure;
+}
+if (!devkit_client_connect(devkit_client, err)) {
+DEBUG0(devkit_client_connect failed);
+goto failure;
+}
+
+/* Populate with known devices.
+ *
+ * This really should be:
+devs = devkit_client_enumerate_by_subsystem(devkit_client, NULL,
err);
+if (err) {
+DEBUG0(devkit_client_enumerate_by_subsystem failed);
+devs = NULL;
+goto failure;
+}
+g_list_foreach(devs, dev_create, devkit_client);
+
Re: [libvirt] RFC: Add a 'reason' argument for domain events ?
On Fri, Nov 14, 2008 at 07:28:09AM -0500, Ben Guthro wrote: To revisit the DEFINED, UNDEFINED topic - I have been giving this some thought, and it seems like it is redundant information to me. Doesn't the existence of a config file define the difference between a transient and a persistent domain? Therefore this could be tracked solely with the enum values we have? If a transient domain becomes persistent via adding a config file, the ADDED event would be emitted, denoting a config file came into being, and by definition changing this domain from transient, to persistent. I'm not sure I understand the need for these events. I see what you mean - if you have tracked a VM through its entire lifecycle you can actually determine whether it was persistent or transient. The missing piece is when you populate your initial list of domains at startup, by calling virConnectListDomainIDs, you don't know which of those initial set of domains is persistent. We can better solve that though by providing an explict API call virDomainIsPersistent(virDomainPtr); So, yes the extra events I suggested for this are redundant. Moving on to the shutdown reason discussion - I can certainly see the need to know why a domain stopped. That said - would it not be more consistent with the API to introduce an event sub-type enum to be passed alongside of the event-type, passed as a second integer? This would reduce the size of the wire protocol, not needing to pass the full character string, as well. This would trivially allow us finer grained notifications within a particular event. A downside is that it may become a bit noisy for clients who may not be interested in all of the sub-events. Perhaps we should add some sort of filtering mechanism? Adding sub-events isn't really making it more chatty, just providing more detail to existing set of events. I gues you could optimize to let people only listen for specific sub-types, but I reckon that domain shutdown events are not frequent enough to justify this extra complexity. I'll have a go a making this adjustment... 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 11/12: virsh support
On Thu, 2008-11-13 at 17:35 +, Daniel P. Berrange wrote:
This patch adds two node virsh commands for the node device enumeration
APIs. The only change here is to change the command names to have a shorter
prefix, nodedev-list and nodedev-dumpxml
Daniel
diff -r 0136f215fc06 src/virsh.c
--- a/src/virsh.c Thu Nov 13 13:06:59 2008 +
+++ b/src/virsh.c Thu Nov 13 13:07:59 2008 +
@@ -4410,6 +4410,96 @@
vshPrint(ctl, _(Running hypervisor: %s %d.%d.%d\n),
hvType, major, minor, rel);
}
+return TRUE;
+}
+
+/*
+ * nodedev-list command
+ */
+static const vshCmdInfo info_node_list_devices[] = {
+{syntax, nodedev-list [--cap capability]},
How about:
+{syntax, nodedev-list [--cap
net|block|storage|scsi|scsi_host|pci|usb]},
so people easily know what capabilities are valid?
+{help, gettext_noop(enumerate devices on this host)},
+{NULL, NULL}
+};
+
+static const vshCmdOptDef opts_node_list_devices[] = {
+{cap, VSH_OT_STRING, VSH_OFLAG_NONE, gettext_noop(capability name)},
+{NULL, 0, 0, NULL}
+};
+
+static int
+cmdNodeListDevices (vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
+{
+char *cap;
+char **devices;
+int found, num_devices, i;
+
+if (!vshConnectionUsability(ctl, ctl-conn, TRUE))
+return FALSE;
+
+cap = vshCommandOptString(cmd, cap, found);
+if (!found)
+cap = NULL;
+
+num_devices = cap ? virNodeNumOfDevicesByCap(ctl-conn, cap, 0) :
+virNodeNumOfDevices(ctl-conn, 0);
+if (num_devices 0) {
+vshError(ctl, FALSE, %s, _(Failed to count node devices));
+return FALSE;
+} else if (num_devices == 0) {
+return TRUE;
+}
+
+devices = vshMalloc(ctl, sizeof(char *) * num_devices);
+num_devices = cap ?
+virNodeListDevicesByCap(ctl-conn, cap, devices, num_devices, 0) :
+virNodeListDevices(ctl-conn, devices, num_devices, 0);
+if (num_devices 0) {
+vshError(ctl, FALSE, %s, _(Failed to list node devices));
+free(devices);
+return FALSE;
+}
+for (i = 0; i num_devices; i++) {
+vshPrint(ctl, %s\n, devices[i]);
Just printing the name makes the output seem a bit sparse - but I guess
the names are fairly descriptive.
@@ -5565,6 +5655,9 @@
{net-uuid, cmdNetworkUuid, opts_network_uuid, info_network_uuid},
{nodeinfo, cmdNodeinfo, NULL, info_nodeinfo},
+{node-list-devices, cmdNodeListDevices, opts_node_list_devices,
info_node_list_devices},
+{node-device-dumpxml, cmdNodeDeviceDumpXML, opts_node_device_dumpxml,
info_node_device_dumpxml},
+
You never actually renamed them to nodedev-list and nodedev-dumpxml
By the way, trying dumpxml on a non-existent name is a bit noisy:
# virsh node-device-dumpxml foo
libvir: Device Monitor error : invalid node device pointer in no node device
with matching name
error: Could not find matching device 'foo'
Cheers,
Mark.
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
On Fri, Nov 14, 2008 at 07:36:27AM -0500, Ben Guthro wrote: I'll answer for Dave, while I'm looking at this. As far as I know, Dave is of the opinion that we are just getting lucky using the APIs as we are, and remains convinced that his suggested change is necessary here. He (and I) remain worried that release of the EventImpl API without this API change could get us into trouble in the future, as we would have to support the released API that has different semantics than DBus, which we were supposed to be modeled closely to. You had sounded convinced it was not necessary the last we heard though... and ultimatley we don't have checkin permissions...so we'll go with whatever you guys decide. Basically, there is no downside to implementing your suggestion of allowing the same FD to be registered, and a clear potential downside to our current impl. So I'll re-write the Add/RemoveHandle API as you suggested to eliminate the risk 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] avoid format string warnings
Daniel Veillard [EMAIL PROTECTED] wrote: Subject: [PATCH] avoid format string warnings * src/openvz_driver.c (ADD_ARG_LIT): Add %s arg before _(...). * src/qemu_driver.c (PCI_ATTACH_OK_MSG): Likewise. * src/util.c (virExec, virRun): Likewise. Oops, sure, +1 ! Committed. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
On Fri, Nov 14, 2008 at 10:46:22AM -0500, David Lively wrote: On Fri, 2008-11-14 at 14:11 +, Daniel P. Berrange wrote: On Fri, Nov 14, 2008 at 07:36:27AM -0500, Ben Guthro wrote: I'll answer for Dave, while I'm looking at this. As far as I know, Dave is of the opinion that we are just getting lucky using the APIs as we are, and remains convinced that his suggested change is necessary here. He (and I) remain worried that release of the EventImpl API without this API change could get us into trouble in the future, as we would have to support the released API that has different semantics than DBus, which we were supposed to be modeled closely to. Yep. Basically, there is no downside to implementing your suggestion of allowing the same FD to be registered, and a clear potential downside to our current impl. So I'll re-write the Add/RemoveHandle API as you suggested to eliminate the risk Thank you! If you'd rather, I'd be happy to make those changes (today) and submit a patch. (But I haven't implemented them yet, other than changing the decls and documentation.) Don't worry, I've already got it in progress myself 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix lv scanning with encrypted volumes
Daniel P. Berrange wrote: On Wed, Nov 12, 2008 at 04:37:16PM -0500, Cole Robinson wrote: See https://bugzilla.redhat.com/show_bug.cgi?id=470693 Using ':' as a delimiter for the lvs command isn't reliable, since it looks like encrypted volume groups have a colon in the physical device name, which throws of the regex. The attached patch changes the delimiter to ','. Maintains existing behavior for me, just waiting on confirmation from the reporter that this indeed does the job. ACK if it fixes the bug. Daniel Okay, reporter confirmed the fix. I've just pushed this. Thanks, Cole -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: OpenVZ bridge support
Sorry for delay.
Big thanks for docs!
This is an update of the patch
http://www.redhat.com/archives/libvir-list/2008-October/msg00326.html
To enable bridge support in the OpenVZ driver. As well as the fixes
suggested last time, it includes an initial bit of HTML doc for the
openvz driver, covering example XML, and the bridge configuration
requirements
Daniel
+
+while(1) {
+if (openvz_readline(fd, line, sizeof(line)) = 0)
+break;
+
+if (!STRPREFIX(line, param)
+line[strlen(param)] == '=') {
+if (safewrite(temp_fd, line, strlen(line)) !=
+strlen(line))
+goto error;
+}
+}
+
this condition make container config completely broken.
as the patch is already commited, here is fix.
diff -u -r1.49 openvz_conf.c
--- openvz_conf.c 12 Nov 2008 16:35:47 - 1.49
+++ openvz_conf.c 14 Nov 2008 16:14:24 -
@@ -484,8 +484,7 @@
if (openvz_readline(fd, line, sizeof(line)) = 0)
break;
-if (!STRPREFIX(line, param)
-line[strlen(param)] == '=') {
+if (!(STRPREFIX(line, param) line[strlen(param)] == '=')) {
if (safewrite(temp_fd, line, strlen(line)) !=
strlen(line))
goto error;
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 0/12: Modular build node devices integration
Daniel P. Berrange [EMAIL PROTECTED] wrote: The following series of patches are updated version of patches 7-11 of this series http://www.redhat.com/archives/libvir-list/2008-October/msg00718.html And integrating David Lively's node device patches ontop Hi Dan, I built that and ran make check under valgrind. There were two leaks in the result, though I'm not sure the leaks are new with this patch series. Here's one of them (the other was similar): 249,136 (1,840 direct, 247,296 indirect) bytes in 115 blocks are definitely lost in loss record 6 of 7 at 0x4A05174: calloc (vg_replace_malloc.c:397) by 0x423C4A: virAlloc (memory.c:100) by 0x426E93: virHashCreate (hash.c:96) by 0x43724C: virGetConnect (datatypes.c:131) by 0x409B64: testCompareFormatXML (xmconfigtest.c:114) by 0x409D7B: testCompareHelper (xmconfigtest.c:172) by 0x40AD66: virtTestRun (testutils.c:92) by 0x409EC9: mymain (xmconfigtest.c:208) by 0x40B426: virtTestMain (testutils.c:443) Here's the fix: From df72657ae1a6d16f1722d1517ec1a1f4ab1e302e Mon Sep 17 00:00:00 2001 From: Jim Meyering [EMAIL PROTECTED] Date: Fri, 14 Nov 2008 18:43:48 +0100 Subject: [PATCH] tests: don't leak connection references * tests/xmconfigtest.c (testCompareFormatXML): Use virUnrefConnect(conn), not VIR_FREE(conn). (testCompareParseXML): Likewise. --- tests/xmconfigtest.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/xmconfigtest.c b/tests/xmconfigtest.c index 276a2e4..b88637f 100644 --- a/tests/xmconfigtest.c +++ b/tests/xmconfigtest.c @@ -93,7 +93,7 @@ static int testCompareParseXML(const char *xmcfg, const char *xml, if (conf) virConfFree(conf); virDomainDefFree(def); -VIR_FREE(conn); +virUnrefConnect(conn); return ret; } @@ -146,7 +146,7 @@ static int testCompareFormatXML(const char *xmcfg, const char *xml, virConfFree(conf); VIR_FREE(gotxml); virDomainDefFree(def); -VIR_FREE(conn); +virUnrefConnect(conn); return ret; } -- 1.6.0.4.911.gc990 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] PATCH: Support events for define/undefine in QEMU
This patch adds support for the DEFINED and UNDEFINED events in the
QEMU drive. This was more involved than I expected, because when the
daemon gets SIGHUP it re-loads config files and we need to broadcast
events for each new config file it finds. So I had to add a callback
to the internal virDomainLoadAllConfigs method.
The LoadConfig method also had a bogus line of code resetting the
domain state to SHUTOFF, which made all your active VMs suddenlly
appear inactive, after the daemon got SIGHUP !
Daniel
diff --git a/src/domain_conf.c b/src/domain_conf.c
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -3242,18 +3242,20 @@ virDomainObjPtr virDomainLoadConfig(virC
virDomainObjListPtr doms,
const char *configDir,
const char *autostartDir,
-const char *name)
+const char *name,
+virDomainLoadConfigNotify notify,
+void *opaque)
{
char *configFile = NULL, *autostartLink = NULL;
virDomainDefPtr def = NULL;
virDomainObjPtr dom;
int autostart;
+int newVM = 1;
if ((configFile = virDomainConfigFile(conn, configDir, name)) == NULL)
goto error;
if ((autostartLink = virDomainConfigFile(conn, autostartDir, name)) ==
NULL)
goto error;
-
if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) 0)
goto error;
@@ -3261,11 +3263,16 @@ virDomainObjPtr virDomainLoadConfig(virC
if (!(def = virDomainDefParseFile(conn, caps, configFile)))
goto error;
+if (virDomainFindByName(doms, def-name))
+newVM = 0;
+
if (!(dom = virDomainAssignDef(conn, doms, def)))
goto error;
-dom-state = VIR_DOMAIN_SHUTOFF;
dom-autostart = autostart;
+
+if (notify)
+(*notify)(dom, newVM, opaque);
return dom;
@@ -3280,7 +3287,9 @@ int virDomainLoadAllConfigs(virConnectPt
virCapsPtr caps,
virDomainObjListPtr doms,
const char *configDir,
-const char *autostartDir)
+const char *autostartDir,
+virDomainLoadConfigNotify notify,
+void *opaque)
{
DIR *dir;
struct dirent *entry;
@@ -3310,7 +3319,9 @@ int virDomainLoadAllConfigs(virConnectPt
doms,
configDir,
autostartDir,
- entry-d_name);
+ entry-d_name,
+ notify,
+ opaque);
if (dom)
dom-persistent = 1;
}
diff --git a/src/domain_conf.h b/src/domain_conf.h
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -549,18 +549,26 @@ int virDomainSaveConfig(virConnectPtr co
const char *configDir,
virDomainDefPtr def);
+typedef void (*virDomainLoadConfigNotify)(virDomainObjPtr dom,
+ int newDomain,
+ void *opaque);
+
virDomainObjPtr virDomainLoadConfig(virConnectPtr conn,
virCapsPtr caps,
virDomainObjListPtr doms,
const char *configDir,
const char *autostartDir,
-const char *name);
+const char *name,
+virDomainLoadConfigNotify notify,
+void *opaque);
int virDomainLoadAllConfigs(virConnectPtr conn,
virCapsPtr caps,
virDomainObjListPtr doms,
const char *configDir,
-const char *autostartDir);
+const char *autostartDir,
+virDomainLoadConfigNotify notify,
+void *opaque);
int virDomainDeleteConfig(virConnectPtr conn,
const char *configDir,
diff --git a/src/lxc_driver.c b/src/lxc_driver.c
--- a/src/lxc_driver.c
+++ b/src/lxc_driver.c
@@ -1017,7 +1017,8 @@ static int lxcStartup(void)
lxc_driver-caps,
lxc_driver-domains,
lxc_driver-configDir,
-lxc_driver-autostartDir) 0) {
+lxc_driver-autostartDir,
+NULL, NULL) 0) {
lxcShutdown();
return -1;
}
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
---
Re: [libvirt] PATCH: Add domain event detail information
Looks pretty much exactly how I pictured it
+1
Daniel P. Berrange wrote on 11/14/2008 12:44 PM:
As per our earlier discussion today, this patch expands the callback for
domain events so that it also gets a event type specific 'detail' field.
This is also kept as an int, and we define enumerations for the possible
values associated with each type. If a event type has no detail, 0 is
passed.
The RESTORED and SAVED event types disappear in this patch and just become
another piece of 'detail' to the STOPPED and STARTED events. I have also
renamed ADDED REMOVED to DEFINED and UNDEFINED to match terminology we
have elsewhere because the names were confusing me
Easiest to just look at the final set:
typedef enum {
VIR_DOMAIN_EVENT_DEFINED = 0,
VIR_DOMAIN_EVENT_UNDEFINED = 1,
VIR_DOMAIN_EVENT_STARTED = 2,
VIR_DOMAIN_EVENT_SUSPENDED = 3,
VIR_DOMAIN_EVENT_RESUMED = 4,
VIR_DOMAIN_EVENT_STOPPED = 5,
} virDomainEventType;
typedef enum {
VIR_DOMAIN_EVENT_STARTED_BOOTED = 0, /* Normal startup from boot */
VIR_DOMAIN_EVENT_STARTED_MIGRATED = 1, /* Incoming migration from another
host */
VIR_DOMAIN_EVENT_STARTED_RESTORED = 2, /* Restored from a state file */
} virDomainEventStartedDetailType;
typedef enum {
VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN = 0, /* Normal shutdown */
VIR_DOMAIN_EVENT_STOPPED_DESTROYED = 1, /* Forced poweroff from host */
VIR_DOMAIN_EVENT_STOPPED_CRASHED = 2, /* Guest crashed */
VIR_DOMAIN_EVENT_STOPPED_MIGRATED = 3, /* Migrated off to another host */
VIR_DOMAIN_EVENT_STOPPED_SAVED = 4, /* Saved to a state file */
VIR_DOMAIN_EVENT_STOPPED_FAILED = 5,/* Host emulator/mgmt failed */
} virDomainEventStoppedDetailType;
typedef enum {
VIR_DOMAIN_EVENT_DEFINED_ADDED = 0, /* Newly created config file */
VIR_DOMAIN_EVENT_DEFINED_UPDATED = 1, /* Changed config file */
} virDomainEventDefinedDetailType;
I don't make use of 'CRASHED' in QEMU driver yet. It might be useful in
Xen though - when a PV guest crashes, Xen stops the domain running, but
leaves it there in a shutoff state, but marked as crashed.
Now using the C event-test program you can see the effects:
myDomainEventCallback1 EVENT: Domain F9x86_64(2) Started Booted
myDomainEventCallback2 EVENT: Domain F9x86_64(2) Started Booted
myDomainEventCallback1 EVENT: Domain F9x86_64(-1) Stopped Destroyed
myDomainEventCallback2 EVENT: Domain F9x86_64(-1) Stopped Destroyed
myDomainEventCallback1 EVENT: Domain F9x86_64(3) Started Booted
myDomainEventCallback2 EVENT: Domain F9x86_64(3) Started Booted
myDomainEventCallback1 EVENT: Domain F9x86_64(3) Suspended
myDomainEventCallback2 EVENT: Domain F9x86_64(3) Suspended
myDomainEventCallback1 EVENT: Domain F9x86_64(3) Resumed
myDomainEventCallback2 EVENT: Domain F9x86_64(3) Resumed
myDomainEventCallback1 EVENT: Domain F9x86_64(-1) Stopped Shutdown
myDomainEventCallback2 EVENT: Domain F9x86_64(-1) Stopped Shutdown
Of the following sequence of actions
virsh start F9x86_64
virsh destroy F9x86_64
virsh start F9x86_64
virsh suspend F9x86_64
virsh resume F9x86_64
virsh shutdown F9x86_64
For the last 'shutdown' operation, you'll see the same if you just run
a graceful shutdown inside the guest itself.
NB, I've not tested saved/restored because my install of KVM is not new
enough to support that correctly, but I expect it to work without trouble.
Likewise for migration.
A word about migration...
- The destination host first gets a STARTED event, with detail MIGRATED
when it starts running
- The source host then gets a STOPPED event with detail MIGRATED when
it completes
- The destination host then gets a RESUMED event, on success, and
a STOPPED event with detail FAILED if migration aborts.
Daniel
examples/domain-events/events-c/event-test.c | 78 +++
examples/domain-events/events-python/event-test.py |8 +-
include/libvirt/libvirt.h | 29 ++-
include/libvirt/libvirt.h.in | 29 ++-
python/libvir.c|6 +
qemud/qemud.h |1
qemud/remote.c | 22 +++--
qemud/remote_protocol.c|2
qemud/remote_protocol.h|1
qemud/remote_protocol.x|1
src/domain_event.c |4 -
src/domain_event.h |6 +
src/qemu_driver.c | 82
+++--
src/remote_internal.c | 29 ---
18 files changed, 366 insertions(+), 118 deletions(-)
diff --git a/examples/domain-events/events-c/event-test.c
b/examples/domain-events/events-c/event-test.c
Re: [libvirt] [PATCH] Java bindings for domain events
On Fri, 2008-11-14 at 17:09 +, Daniel P. Berrange wrote:
On Fri, Nov 14, 2008 at 12:00:10PM -0500, David Lively wrote:
+JNIEXPORT void JNICALL Java_org_libvirt_Connect_registerForDomainEvents
+(JNIEnv *env, jobject obj, jlong VCP){
+// TODO: Need to DeleteGlobalRef(obj) when deregistering for
callbacks.
+// But then need to track global obj per Connect object.
Hum, that's a bit nasty. Can we make sure we can plug the leaks
without having to change the APIs, that would be a bummer...
Yeah. It's really not acceptable as is. The easiest solution (as you
hint) is changing the API so virConnectDomainEventDeregister returns the
void * registered with that callback. That would (of course) be my
preference. What do you think? That API hasn't been released quite
yet ...
Or have the virConnectDomainEventRegister method take an extra parameter
which is a callbackvoid (*freefunc)(void*). libvirt would just invoke
that to free the opaque data chunk.
Yeah, I like this better. The dbus(?) API allows an optional
destructor (freefunc) to be specified for callback userdata. So let's
allow it to be null (in which case we obviously don't call it at remove
time).
I think we need a similar thing with the event loops APIs for timers
and file handle watches, to make it easier to free the opaque data
blob they have.
Sounds good too.
I can make the DomainEvent changes today / this weekend while working on
the Java bindings (since I need them to plug the Java leak), and submit
them on Monday (or perhaps later today, if I don't get diverted).
Are you going to make the event impl changes?
Thanks,
Dave
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 8/12:Interal driver API for node devices
On Fri, 2008-11-14 at 13:20 +, Mark McLoughlin wrote:
On Thu, 2008-11-13 at 17:31 +, Daniel P. Berrange wrote:
+virBufferVSprintf(buf, capability type='%s'\n,
+ virNodeDevCapTypeToString(caps-type));
+switch (caps-type) {
...
+case VIR_NODE_DEV_CAP_NET:
+virBufferVSprintf(buf, interface%s/interface\n,
+ data-net.interface);
+if (data-net.address)
+virBufferVSprintf(buf, address%s/address\n,
+ data-net.address);
+if (data-net.subtype != VIR_NODE_DEV_CAP_NET_LAST) {
+const char *subtyp =
+virNodeDevNetCapTypeToString(data-net.subtype);
+virBufferVSprintf(buf, capability type='%s'\n,
subtyp);
+switch (data-net.subtype) {
+case VIR_NODE_DEV_CAP_NET_80203:
+virBufferVSprintf(buf,
+
mac_address%012llx/address\n,
+
data-net.data.ieee80203.mac_address);
+break;
+case VIR_NODE_DEV_CAP_NET_80211:
+virBufferVSprintf(buf,
+
mac_address%012llx/address\n,
+
data-net.data.ieee80211.mac_address);
+break;
+case VIR_NODE_DEV_CAP_NET_LAST:
+/* Keep dumb compiler happy */
+break;
+}
+virBufferAddLit(buf, /capability\n);
This all seems a bit odd.
We've listed the mac address once already, so why do it again in a hex format?
Agreed. It's basically just reflecting the HAL attributes. But I have
to admit I find a lot of HAL rather non-intuitive ... I have no
particular attachment to any of this node device description XML. I
really liked Daniel's suggestion that this be based on a subset of HAL,
and have tried to do that as much as possible. I don't really care what
it's based on, but it sure would be nice to avoid inventing yet another
XML device representation (hence the attraction to HAL).
But the more I've gotten to know HAL, the less I've liked the idea. I'm
not aware of an alternative that doesn't involve inventing our own.
Perhaps there's something suitable in CIM?? I'm a CIM-dummy, so I have
no idea ... This is related to the device naming issue as well. I
share all of Dan B's concerns here ...
And I wouldn't think of 802.3 vs 802.11 as a network device capability, but
more like it's type - they're mutually exclusive, right?
Also, we have:
device
...
capability type='net'
...
capability type='80203'
mac_address001320f74a06/address
/capability
/capability
/device
i.e. two different capability semantics?
Again, just reflecting HAL. But I agree it seems bizarre.
+}
+break;
+case VIR_NODE_DEV_CAP_BLOCK:
+virBufferVSprintf(buf, device%s/device\n,
+ data-block.device);
Two nested device nodes:
device
...
capability type='block'
device/dev/sda1/device
/capability
/device
How about path or device_path ?
This one doesn't bother me so much, though granted the two device tags
are talking about completely different things (clear to me from the
context, but perhaps violating some proper XML design rules?).
+if (data-storage.originating_device)
+virBufferVSprintf(buf,
+ originating_device%s
+ /originating_device\n,
+ data-storage.originating_device);
This originating_device thing sounds strange, and I don't think we
implement it yet. Leave it out for now?
Fine with me. I don't know what it is -- just sounded generally useful.
If it's not widely implemented, we probably shouldn't bother supporting
it.
+if (data-storage.flags VIR_NODE_DEV_CAP_STORAGE_REMOVABLE) {
+int avl = data-storage.flags
+VIR_NODE_DEV_CAP_STORAGE_REMOVABLE_MEDIA_AVAILABLE;
+virBufferAddLit(buf, capability
type='removable'\n);
+virBufferVSprintf(buf,
+media_available%d
+ /media_available\n, avl ? 1 : 0);
+virBufferVSprintf(buf,
media_size%llu/media_size\n,
+ data-storage.removable_media_size);
+virBufferAddLit(buf, /capability\n);
+} else {
+virBufferVSprintf(buf, size%llu/size\n,
+ data-storage.size);
+
Re: [libvirt] PATCH: 7/12: Public API for node devices
On Fri, 2008-11-14 at 13:28 +, Daniel P. Berrange wrote: On Fri, Nov 14, 2008 at 12:46:09PM +, Mark McLoughlin wrote: On Thu, 2008-11-13 at 17:30 +, Daniel P. Berrange wrote: This patch is the public API parts of the node device enumeration code. No changes since David's last submission of this, except for some Makefile tweaks + +int virNodeNumOfDevices (virConnectPtr conn, + unsigned int flags); + +int virNodeListDevices (virConnectPtr conn, + char **const names, + int maxnames, + unsigned int flags); + +int virNodeNumOfDevicesByCap (virConnectPtr conn, + const char *cap, + unsigned int flags); + +int virNodeListDevicesByCap (virConnectPtr conn, + const char *cap, + char **const names, + int maxnames, + unsigned int flags); How about combining these two sets of functions and if the capability type isn't supplied list all devices? Yes, we could just remove the ByCap APIs, and add the 'const char *cap' arg to the first two APIs, allowing NULL. I like this idea as well. + +virNodeDevicePtrvirNodeDeviceLookupByName (virConnectPtr conn, + const char *name); + +const char *virNodeDeviceGetName (virNodeDevicePtr dev); + How stable are these names? e.g. should we say that no-one should rely on the format of the name and that the name of a given device could change across node reboots? Even if HAL guarantees the name to be stable (does it?), if you switch between HAL and DevKit it could change, right? I don't think HAL explicitly guarentees it, it merely happens to have been stable AFAICT. The naming is definitely completely different between HAL and DevKit. This is probably my biggest worry with the impl so far - some app using it will need to have a stable identifier for a device and we won't be providing it. We could invent our own stable naming scheme for devices - the scheme would vary per capability - eg for PCI devices we can use the bus, function, slot identifiers. USB is hard to guarentee though - if a device is plugged in unpluged plugged in again it won't get the same address, and there's no real other identifier we can rely on for this. Yep. But I think HAL is already trying to specify stable names wherever possible. E.g., PCI devs aren't named by position on bus, but by vendor-id/product-id, so that a PCI impl supporting hotplug will give the same name for the card if it's taken out and reinserted (well, more or less ...). Perhaps we could just identify where we think HAL gets it wrong, and implement our own naming scheme for those devices (assuming we can come up with one we like better), and using the HAL names for the rest?? I see that all this naturally flows from the way hal does things, but the pci device isn't a parent of the network device in any real sense ... I guess I would expect to just see one device with both the 'net' and 'pci' capabilities. So that's basically removing all explicit tracking of 'logical' devices (NICs, disk, etc) and only ever representing 'physical' devices (PCI, USB devices). The problem I can see is that one physical device can result in many logical devices - even multiple instances of the same capability - particularly multi-function USB devices can result in a large number of sub-devices for audio, video, etc. Or a SCSI host adapter can have a single PCI device, but result in multiple host adapters in the OS view. Separating the physical from logical devices gives us the opportunity to define more stable names for devices with certain capabilities. eg, for a USB network card, its hard to invent a stable name at the level of the USB device, but for the logical NIC you can easily invent a name based off the MAC address. Agreed. I think this extra level of heirarchy is useful, though I originally found it confusing. +int virNodeDeviceNumOfCaps (virNodeDevicePtr dev); + +int virNodeDeviceListCaps(virNodeDevicePtr dev, + char **const names, + int maxnames); I think it would make more sense to me if there was a fixed set of capability types and for them to be an enum in the API
Re: [libvirt] PATCH: 9/12: HAL/DevitKit node device impl
On Fri, 2008-11-14 at 13:41 +, Mark McLoughlin wrote:
The DeviceKit implementation seems to be much more of a work-in-progress
than the HAL implementation - maybe disable it by default in configure?
(i.e. with_devkit=no instead of with_devkit=check)
That's a good idea. I don't think the devkit impl can go forward until
devkit itself starts moving forward. (Hmmm ... I (now) see the devkit
mailing list has started getting some traffic. Apparently they've just
released v2. Perhaps it's time for another look, though I can't sign up
for that right now.)
+interface = strrchr(sysfs_path, '/');
+if (!interface *interface *(++interface))
+return -1;
Was this meant to be:
if (!interface || !*interface || !*(++interface))
Yes. Or the equivalent (! (interface *interface *(++interface)).
+static void dev_create(void *_dkdev, void *_dkclient ATTRIBUTE_UNUSED)
+{
...
+
+dev-privateData = dkdev;
You need need a privateFree() hook here to do g_object_unref(), I think
Yes. Good catch, again.
+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);
Need a g_list_free() here right?
Yes ...
+}
+
+driverState-privateData = devkit_client;
+
+// TODO: Register to get DeviceKit events on device changes and
+// coordinate updates with queries and other operations.
That's a pretty big missing feature - if both HAL and DevKit were
available on a system, we'd still want HAL until this is fixed, right?
Absolutely. I think this supports your request that this be disabled by
default.
+(void)get_int_prop(ctx, udi, pci.vendor_id, (int
*)d-pci_dev.vendor);
+if (get_str_prop(ctx, udi, pci.vendor, d-pci_dev.vendor_name) != 0)
+(void)get_str_prop(ctx, udi, info.vendor,
d-pci_dev.vendor_name);
+(void)get_int_prop(ctx, udi, pci.product_id, (int
*)d-pci_dev.product);
+if (get_str_prop(ctx, udi, pci.product, d-pci_dev.product_name) !=
0)
+(void)get_str_prop(ctx, udi, info.product,
d-pci_dev.product_name);
By the way - vendor and product IDs are normally quoted in hex, not
decimal - e.g. I'd know my NIC is 8086:10de, not 32902:4318
I guess most other integer values in libvirt XML are decimal, but might
be worth adding a hex format for this?
I'd prefer hex for vid/pid as well; I just stuck with decimal since the
rest of libvirt does. If this does get changed to output hex, I'd only
request that we prefix hex numbers with 0x so people don't have to
remember which attrs are dumped in hex and which in decimal.
+static void device_prop_modified(LibHalContext *ctx ATTRIBUTE_UNUSED,
+ const char *udi,
+ const char *key,
+ dbus_bool_t is_removed ATTRIBUTE_UNUSED,
+ dbus_bool_t is_added ATTRIBUTE_UNUSED)
+{
+const char *name = hal_name(udi);
+virNodeDeviceObjPtr dev =
virNodeDeviceFindByName(driverState-devs,name);
+DEBUG(%s %s, key, name);
+if (dev) {
+/* Simply rediscover device -- incrementally handling changes
+ * to properties (which are mapped into caps in very capability-
+ * specific ways) is nasty ... so avoid it.
+ */
+virNodeDeviceObjRemove(driverState-devs, dev);
+dev_create(strdup(udi));
+} else
+DEBUG(no device named %s, name);
+}
Could use the same callback for both of these (with a cast), or simplify
them to:
static void device_prop_modified(LibHalContext *ctx ATTRIBUTE_UNUSED,
const char *udi,
const char *key,
dbus_bool_t is_removed ATTRIBUTE_UNUSED,
dbus_bool_t is_added ATTRIBUTE_UNUSED)
{
DEBUG(%s %s, key, name);
/* Simply rediscover device -- incrementally handling changes
* to properties (which are mapped into caps in very capability-
* specific ways) is nasty ... so avoid it.
*/
device_removed(ctx, udi);
device_added(ctx, udi);
}
Yep, that's a good idea.
Thanks for the careful review,
Dave
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
