[libvirt] [PATCH] events: Avoid double free possibility on remote call failure
by John Ferlan
If a remote call fails during event registration (more than likely from
a network failure or remote libvirtd restart timed just right), then when
calling the virObjectEventStateDeregisterID we don't want to call the
registered @freecb function because that breaks our contract that we
would only call it after succesfully returning. If the @freecb routine
were called, it could result in a double free from properly coded
applications that free their opaque data on failure to register, as seen
in the following details:
Program terminated with signal 6, Aborted.
#0 0x00007fc45cba15d7 in raise
#1 0x00007fc45cba2cc8 in abort
#2 0x00007fc45cbe12f7 in __libc_message
#3 0x00007fc45cbe86d3 in _int_free
#4 0x00007fc45d8d292c in PyDict_Fini
#5 0x00007fc45d94f46a in Py_Finalize
#6 0x00007fc45d960735 in Py_Main
#7 0x00007fc45cb8daf5 in __libc_start_main
#8 0x0000000000400721 in _start
The double dereference of 'pyobj_cbData' is triggered in the following way:
(1) libvirt_virConnectDomainEventRegisterAny is invoked.
(2) the event is successfully added to the event callback list
(virDomainEventStateRegisterClient in
remoteConnectDomainEventRegisterAny returns 1 which means ok).
(3) when function remoteConnectDomainEventRegisterAny is hit,
network connection disconnected coincidently (or libvirtd is
restarted) in the context of function 'call' then the connection
is lost and the function 'call' failed, the branch
virObjectEventStateDeregisterID is therefore taken.
(4) 'pyobj_conn' is dereferenced the 1st time in
libvirt_virConnectDomainEventFreeFunc.
(5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
2nd time in libvirt_virConnectDomainEventRegisterAny.
(6) the double free error is triggered.
Resolve this by adding an @inhibitFreeCb boolean in order to avoid calling
freecb in virObjectEventStateDeregisterID for any remote call failure in
a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls,
the passed value would be false indicating they should run the freecb if it
exists.
Patch based on the investigation and initial patch posted by:
fangying <fangying1(a)huawei.com>.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
Initial patch found at:
https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html
based on feedback from Daniel Berrange to a previous posting:
https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html
Since no new patch was posted, I posted my idea from review for
consideration.
src/bhyve/bhyve_driver.c | 2 +-
src/conf/domain_event.c | 2 +-
src/conf/object_event.c | 23 +++++++++++++++++------
src/conf/object_event.h | 3 ++-
src/libxl/libxl_driver.c | 2 +-
src/lxc/lxc_driver.c | 2 +-
src/network/bridge_driver.c | 2 +-
src/node_device/node_device_driver.c | 2 +-
src/qemu/qemu_driver.c | 4 ++--
src/remote/remote_driver.c | 32 ++++++++++++++++----------------
src/secret/secret_driver.c | 2 +-
src/storage/storage_driver.c | 2 +-
src/test/test_driver.c | 8 ++++----
src/uml/uml_driver.c | 2 +-
src/vz/vz_driver.c | 2 +-
src/xen/xen_driver.c | 2 +-
16 files changed, 52 insertions(+), 40 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index ed2221a..6722dc4 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1503,7 +1503,7 @@ bhyveConnectDomainEventDeregisterAny(virConnectPtr conn,
if (virObjectEventStateDeregisterID(conn,
privconn->domainEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
return -1;
return 0;
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 6e471d7..ff4c602 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -2301,7 +2301,7 @@ virDomainEventStateDeregister(virConnectPtr conn,
NULL);
if (callbackID < 0)
return -1;
- return virObjectEventStateDeregisterID(conn, state, callbackID);
+ return virObjectEventStateDeregisterID(conn, state, callbackID, false);
}
diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index e5f942f..5e944af 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -234,13 +234,15 @@ virObjectEventCallbackListCount(virConnectPtr conn,
* @conn: pointer to the connection
* @cbList: the list
* @callback: the callback to remove
+ * @inhibitFreeCb: Inhibit calling the freecb
*
* Internal function to remove a callback from a virObjectEventCallbackListPtr
*/
static int
virObjectEventCallbackListRemoveID(virConnectPtr conn,
virObjectEventCallbackListPtr cbList,
- int callbackID)
+ int callbackID,
+ bool inhibitFreeCb)
{
size_t i;
@@ -256,7 +258,10 @@ virObjectEventCallbackListRemoveID(virConnectPtr conn,
cb->key_filter ? cb->key : NULL,
cb->remoteID >= 0) - 1);
- if (cb->freecb)
+ /* inhibiting calling @freecb from error paths in register
+ * functions ensures the caller of the register function won't
+ * end up with a double free error */
+ if (!inhibitFreeCb && cb->freecb)
(*cb->freecb)(cb->opaque);
virObjectEventCallbackFree(cb);
VIR_DELETE_ELEMENT(cbList->callbacks, i, cbList->count);
@@ -927,16 +932,22 @@ virObjectEventStateRegisterID(virConnectPtr conn,
* @conn: connection to associate with callback
* @state: object event state
* @callbackID: ID of the function to remove from event
+ * @inhibitFreeCb: Inhibit the calling of a freecb
*
* Unregister the function @callbackID with connection @conn,
- * from @state, for events.
+ * from @state, for events. If @inhibitFreeCb is true, then we
+ * are being called from a remote call failure path for the
+ * Event registration indicating a -1 return to the caller. The
+ * caller wouldn't expect us to run their freecb function if it
+ * exists, so we cannot do so.
*
* Returns: the number of callbacks still registered, or -1 on error
*/
int
virObjectEventStateDeregisterID(virConnectPtr conn,
virObjectEventStatePtr state,
- int callbackID)
+ int callbackID,
+ bool inhibitFreeCb)
{
int ret;
@@ -946,8 +957,8 @@ virObjectEventStateDeregisterID(virConnectPtr conn,
state->callbacks,
callbackID);
else
- ret = virObjectEventCallbackListRemoveID(conn,
- state->callbacks, callbackID);
+ ret = virObjectEventCallbackListRemoveID(conn, state->callbacks,
+ callbackID, inhibitFreeCb);
virObjectEventStateCleanupTimer(state, true);
diff --git a/src/conf/object_event.h b/src/conf/object_event.h
index 7a9995e..b36fcc9 100644
--- a/src/conf/object_event.h
+++ b/src/conf/object_event.h
@@ -73,7 +73,8 @@ virObjectEventStateQueueRemote(virObjectEventStatePtr state,
int
virObjectEventStateDeregisterID(virConnectPtr conn,
virObjectEventStatePtr state,
- int callbackID)
+ int callbackID,
+ bool inhibitFreeCb)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 0d65342..2995324 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5652,7 +5652,7 @@ libxlConnectDomainEventDeregisterAny(virConnectPtr conn, int callbackID)
if (virObjectEventStateDeregisterID(conn,
driver->domainEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
return -1;
return 0;
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 22c8b58..ec357c6 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1479,7 +1479,7 @@ lxcConnectDomainEventDeregisterAny(virConnectPtr conn,
if (virObjectEventStateDeregisterID(conn,
driver->domainEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
return -1;
return 0;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3ba7018..e9ad77e 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3001,7 +3001,7 @@ networkConnectNetworkEventDeregisterAny(virConnectPtr conn,
if (virObjectEventStateDeregisterID(conn,
driver->networkEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 760d73a..37e4e19 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -690,7 +690,7 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
if (virObjectEventStateDeregisterID(conn,
driver->nodeDeviceEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e91663c..8c10599 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11826,7 +11826,7 @@ qemuConnectDomainEventDeregisterAny(virConnectPtr conn,
if (virObjectEventStateDeregisterID(conn,
driver->domainEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
goto cleanup;
ret = 0;
@@ -18472,7 +18472,7 @@ qemuConnectDomainQemuMonitorEventDeregister(virConnectPtr conn,
goto cleanup;
if (virObjectEventStateDeregisterID(conn, driver->domainEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index b452e8b..6ebf910 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -3066,7 +3066,7 @@ remoteConnectNetworkEventRegisterAny(virConnectPtr conn,
(xdrproc_t) xdr_remote_connect_network_event_register_any_args, (char *) &args,
(xdrproc_t) xdr_remote_connect_network_event_register_any_ret, (char *) &ret) == -1) {
virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID);
+ callbackID, true);
goto done;
}
virObjectEventStateSetRemote(conn, priv->eventState, callbackID,
@@ -3099,7 +3099,7 @@ remoteConnectNetworkEventDeregisterAny(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 0)
goto done;
/* If that was the last callback for this eventID, we need to disable
@@ -3160,7 +3160,7 @@ remoteConnectStoragePoolEventRegisterAny(virConnectPtr conn,
(xdrproc_t) xdr_remote_connect_storage_pool_event_register_any_args, (char *) &args,
(xdrproc_t) xdr_remote_connect_storage_pool_event_register_any_ret, (char *) &ret) == -1) {
virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID);
+ callbackID, true);
goto done;
}
@@ -3193,7 +3193,7 @@ remoteConnectStoragePoolEventDeregisterAny(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 0)
goto done;
/* If that was the last callback for this eventID, we need to disable
@@ -3256,7 +3256,7 @@ remoteConnectNodeDeviceEventRegisterAny(virConnectPtr conn,
(xdrproc_t) xdr_remote_connect_node_device_event_register_any_args, (char *) &args,
(xdrproc_t) xdr_remote_connect_node_device_event_register_any_ret, (char *) &ret) == -1) {
virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID);
+ callbackID, true);
goto done;
}
@@ -3290,7 +3290,7 @@ remoteConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 0)
goto done;
/* If that was the last callback for this eventID, we need to disable
@@ -3353,7 +3353,7 @@ remoteConnectSecretEventRegisterAny(virConnectPtr conn,
(xdrproc_t) xdr_remote_connect_secret_event_register_any_args, (char *) &args,
(xdrproc_t) xdr_remote_connect_secret_event_register_any_ret, (char *) &ret) == -1) {
virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID);
+ callbackID, true);
goto done;
}
@@ -3387,7 +3387,7 @@ remoteConnectSecretEventDeregisterAny(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 0)
goto done;
/* If that was the last callback for this eventID, we need to disable
@@ -3453,7 +3453,7 @@ remoteConnectDomainQemuMonitorEventRegister(virConnectPtr conn,
(xdrproc_t) xdr_qemu_connect_domain_monitor_event_register_args, (char *) &args,
(xdrproc_t) xdr_qemu_connect_domain_monitor_event_register_ret, (char *) &ret) == -1) {
virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID);
+ callbackID, true);
goto done;
}
virObjectEventStateSetRemote(conn, priv->eventState, callbackID,
@@ -3485,7 +3485,7 @@ remoteConnectDomainQemuMonitorEventDeregister(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 0)
goto done;
/* If that was the last callback for this event, we need to disable
@@ -4409,7 +4409,7 @@ remoteConnectDomainEventRegister(virConnectPtr conn,
(xdrproc_t) xdr_remote_connect_domain_event_callback_register_any_args, (char *) &args,
(xdrproc_t) xdr_remote_connect_domain_event_callback_register_any_ret, (char *) &ret) == -1) {
virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID);
+ callbackID, true);
goto done;
}
virObjectEventStateSetRemote(conn, priv->eventState, callbackID,
@@ -4419,7 +4419,7 @@ remoteConnectDomainEventRegister(virConnectPtr conn,
(xdrproc_t) xdr_void, (char *) NULL,
(xdrproc_t) xdr_void, (char *) NULL) == -1) {
virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID);
+ callbackID, true);
goto done;
}
}
@@ -4452,7 +4452,7 @@ remoteConnectDomainEventDeregister(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 0)
goto done;
if (count == 0) {
@@ -5951,7 +5951,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
(xdrproc_t) xdr_remote_connect_domain_event_callback_register_any_args, (char *) &args,
(xdrproc_t) xdr_remote_connect_domain_event_callback_register_any_ret, (char *) &ret) == -1) {
virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID);
+ callbackID, true);
goto done;
}
virObjectEventStateSetRemote(conn, priv->eventState, callbackID,
@@ -5965,7 +5965,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
(xdrproc_t) xdr_remote_connect_domain_event_register_any_args, (char *) &args,
(xdrproc_t) xdr_void, (char *)NULL) == -1) {
virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID);
+ callbackID, true);
goto done;
}
}
@@ -5996,7 +5996,7 @@ remoteConnectDomainEventDeregisterAny(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 0)
goto done;
/* If that was the last callback for this eventID, we need to disable
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index fc01e6d..7d7ded8 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -532,7 +532,7 @@ secretConnectSecretEventDeregisterAny(virConnectPtr conn,
if (virObjectEventStateDeregisterID(conn,
driver->secretEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ab1dc8f..698654c 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2662,7 +2662,7 @@ storageConnectStoragePoolEventDeregisterAny(virConnectPtr conn,
if (virObjectEventStateDeregisterID(conn,
driver->storageEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 11e7fd8..fec4d83 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5697,7 +5697,7 @@ testConnectDomainEventDeregisterAny(virConnectPtr conn,
int ret = 0;
if (virObjectEventStateDeregisterID(conn, driver->eventState,
- callbackID) < 0)
+ callbackID, false) < 0)
ret = -1;
return ret;
@@ -5731,7 +5731,7 @@ testConnectNetworkEventDeregisterAny(virConnectPtr conn,
int ret = 0;
if (virObjectEventStateDeregisterID(conn, driver->eventState,
- callbackID) < 0)
+ callbackID, false) < 0)
ret = -1;
return ret;
@@ -5764,7 +5764,7 @@ testConnectStoragePoolEventDeregisterAny(virConnectPtr conn,
int ret = 0;
if (virObjectEventStateDeregisterID(conn, driver->eventState,
- callbackID) < 0)
+ callbackID, false) < 0)
ret = -1;
return ret;
@@ -5797,7 +5797,7 @@ testConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
int ret = 0;
if (virObjectEventStateDeregisterID(conn, driver->eventState,
- callbackID) < 0)
+ callbackID, false) < 0)
ret = -1;
return ret;
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 080fea4..c3dc9d2 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -2721,7 +2721,7 @@ umlConnectDomainEventDeregisterAny(virConnectPtr conn,
umlDriverLock(driver);
if (virObjectEventStateDeregisterID(conn,
driver->domainEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
ret = -1;
umlDriverUnlock(driver);
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 7aa0c4c..1fa2e2f 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -1053,7 +1053,7 @@ vzConnectDomainEventDeregisterAny(virConnectPtr conn,
if (virObjectEventStateDeregisterID(conn,
privconn->driver->domainEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
return -1;
return 0;
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 8e7bc35..f6d7fdc 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -2284,7 +2284,7 @@ xenUnifiedConnectDomainEventDeregisterAny(virConnectPtr conn,
if (virObjectEventStateDeregisterID(conn,
priv->domainEvents,
- callbackID) < 0)
+ callbackID, false) < 0)
ret = -1;
xenUnifiedUnlock(priv);
--
2.9.4
7 years, 9 months
[libvirt] [PATCH] Improve logging of shutdown inhibitor
by Daniel P. Berrange
The log category for virnetdaemon.c was mistakenly set
to rpc.netserver. Some useful info about the inhibitor
file descriptor was also never logged.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/rpc/virnetdaemon.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 782417e..905cecc 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -47,7 +47,7 @@
#define VIR_FROM_THIS VIR_FROM_RPC
-VIR_LOG_INIT("rpc.netserver");
+VIR_LOG_INIT("rpc.netdaemon");
typedef struct _virNetDaemonSignal virNetDaemonSignal;
typedef virNetDaemonSignal *virNetDaemonSignalPtr;
@@ -460,9 +460,11 @@ virNetDaemonGotInhibitReply(DBusPendingCall *pending,
DBUS_TYPE_INVALID)) {
if (dmn->autoShutdownInhibitions) {
dmn->autoShutdownInhibitFd = fd;
+ VIR_DEBUG("Got inhibit FD %d", fd);
} else {
/* We stopped the last VM since we made the inhibit call */
VIR_FORCE_CLOSE(fd);
+ VIR_DEBUG("Closing inhibit FD %d", fd);
}
}
virDBusMessageUnref(reply);
@@ -550,8 +552,10 @@ virNetDaemonRemoveShutdownInhibition(virNetDaemonPtr dmn)
VIR_DEBUG("dmn=%p inhibitions=%zu", dmn, dmn->autoShutdownInhibitions);
- if (dmn->autoShutdownInhibitions == 0)
+ if (dmn->autoShutdownInhibitions == 0) {
+ VIR_DEBUG("Closing inhibit FD %d", dmn->autoShutdownInhibitFd);
VIR_FORCE_CLOSE(dmn->autoShutdownInhibitFd);
+ }
virObjectUnlock(dmn);
}
--
2.9.3
7 years, 9 months
[libvirt] [PATCH] Fix conditional check for DBus
by Daniel P. Berrange
The DBus conditional was renamed way back:
commit da77f04ed5fa0731d50b947be8c739bdbf8121ad
Author: Daniel P. Berrange <berrange(a)redhat.com>
Date: Thu Sep 20 15:05:39 2012 +0100
Convert HAVE_DBUS to WITH_DBUS
but the shutdown inhibit code was not updated. Thus libvirt
was never inhibiting shutdown by a logged in user when VMs
are running.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
daemon/libvirtd.c | 4 ++--
src/rpc/virnetdaemon.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index db239f0..a558458 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -795,7 +795,7 @@ static void daemonInhibitCallback(bool inhibit, void *opaque)
}
-#ifdef HAVE_DBUS
+#ifdef WITH_DBUS
static DBusConnection *sessionBus;
static DBusConnection *systemBus;
@@ -887,7 +887,7 @@ static void daemonRunStateInit(void *opaque)
driversInitialized = true;
-#ifdef HAVE_DBUS
+#ifdef WITH_DBUS
/* Tie the non-privileged libvirtd to the session/shutdown lifecycle */
if (!virNetDaemonIsPrivileged(dmn)) {
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index fabacf2..782417e 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -437,7 +437,7 @@ virNetDaemonAutoShutdown(virNetDaemonPtr dmn,
}
-#if defined(HAVE_DBUS) && defined(DBUS_TYPE_UNIX_FD)
+#if defined(WITH_DBUS) && defined(DBUS_TYPE_UNIX_FD)
static void
virNetDaemonGotInhibitReply(DBusPendingCall *pending,
void *opaque)
@@ -529,7 +529,7 @@ virNetDaemonAddShutdownInhibition(virNetDaemonPtr dmn)
VIR_DEBUG("dmn=%p inhibitions=%zu", dmn, dmn->autoShutdownInhibitions);
-#if defined(HAVE_DBUS) && defined(DBUS_TYPE_UNIX_FD)
+#if defined(WITH_DBUS) && defined(DBUS_TYPE_UNIX_FD)
if (dmn->autoShutdownInhibitions == 1)
virNetDaemonCallInhibit(dmn,
"shutdown",
--
2.9.3
7 years, 9 months
[libvirt] [PATCH v2 0/2] qemu: allow hotplugging disks with duplicate WWNs
by Peter Krempa
v2 contains a test as requested.
Peter Krempa (2):
Revert "qemu: Check duplicate WWNs also for hotplugged disks"
tests: hotplug: Test disks with duplicate WWNs
src/conf/domain_conf.c | 37 -----------
tests/qemuhotplugtest.c | 5 ++
.../qemuhotplug-disk-scsi-duplicate-wwn.xml | 8 +++
...-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml | 74 ++++++++++++++++++++++
.../qemuhotplug-base-live+disk-scsi-wwn.xml | 63 ++++++++++++++++++
5 files changed, 150 insertions(+), 37 deletions(-)
create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-duplicate-wwn.xml
create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml
create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-wwn.xml
--
2.12.2
7 years, 9 months
[libvirt] [PATCH] util: Extract locale-related fixes into separate functions
by Martin Kletzander
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/util/virstring.c | 96 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 60 insertions(+), 36 deletions(-)
diff --git a/src/util/virstring.c b/src/util/virstring.c
index feea5be05198..6125725364f3 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -522,6 +522,7 @@ virStrToLong_ullp(char const *s, char **end_ptr, int base,
#if HAVE_NEWLOCALE
static locale_t virLocale;
+static locale_t virLocaleOld;
static int
virLocaleOnceInit(void)
@@ -533,7 +534,58 @@ virLocaleOnceInit(void)
}
VIR_ONCE_GLOBAL_INIT(virLocale);
-#endif
+
+static int
+virLocaleSet(void)
+{
+ if (virLocaleInitialize() < 0)
+ return -1;
+ virLocaleOld = uselocale(virLocale);
+ return 0;
+}
+
+static void
+virLocaleRevert(void)
+{
+ uselocale(virLocaleOld);
+}
+
+static void
+virLocaleFixupRadix(char **strp ATTRIBUTE_UNUSED)
+{
+}
+
+#else /* !HAVE_NEWLOCALE */
+
+static int
+virLocaleSet(void)
+{
+ return 0;
+}
+
+static void
+virLocaleRevert(void)
+{
+}
+
+static void
+virLocaleFixupRadix(char **strp)
+{
+ char *radix, *tmp;
+ struct lconv *lc;
+
+ lc = localeconv();
+ radix = lc->decimal_point;
+ tmp = strstr(*strp, radix);
+ if (tmp) {
+ *tmp = '.';
+ if (strlen(radix) > 1)
+ memmove(tmp + 1, tmp + strlen(radix), strlen(*strp) - (tmp - *strp));
+ }
+}
+
+#endif /* !HAVE_NEWLOCALE */
+
/**
* virStrToDouble
@@ -552,17 +604,11 @@ virStrToDouble(char const *s,
int err;
errno = 0;
-#if HAVE_NEWLOCALE
- locale_t old_loc;
- if (virLocaleInitialize() < 0)
+ if (virLocaleSet() < 0)
return -1;
-
- old_loc = uselocale(virLocale);
-#endif
val = strtod(s, &p); /* exempt from syntax-check */
-#if HAVE_NEWLOCALE
- uselocale(old_loc);
-#endif
+ virLocaleRevert();
+
err = (errno || (!end_ptr && *p) || p == s);
if (end_ptr)
*end_ptr = p;
@@ -584,36 +630,14 @@ virDoubleToStr(char **strp, double number)
{
int ret = -1;
-#if HAVE_NEWLOCALE
-
- locale_t old_loc;
-
- if (virLocaleInitialize() < 0)
- goto error;
+ if (virLocaleSet() < 0)
+ return -1;
- old_loc = uselocale(virLocale);
ret = virAsprintf(strp, "%lf", number);
- uselocale(old_loc);
-
-#else
-
- char *radix, *tmp;
- struct lconv *lc;
-
- if ((ret = virAsprintf(strp, "%lf", number) < 0))
- goto error;
- lc = localeconv();
- radix = lc->decimal_point;
- tmp = strstr(*strp, radix);
- if (tmp) {
- *tmp = '.';
- if (strlen(radix) > 1)
- memmove(tmp + 1, tmp + strlen(radix), strlen(*strp) - (tmp - *strp));
- }
+ virLocaleRevert();
+ virLocaleFixupRadix(strp);
-#endif /* HAVE_NEWLOCALE */
- error:
return ret;
}
--
2.13.1
7 years, 9 months
[libvirt] [PATCH] virNetDevOpenvswitchInterfaceStats: Be more forgiving when fetching stats
by Michal Privoznik
https://bugzilla.redhat.com/show_bug.cgi?id=1461270
When fetching stats for a vhost-user type of interface, we run
couple of ovs-vsctl commands and parse their output. However, not
all stats exist at all times, for instance "rx_dropped" or
"tx_errors" can be missing. Thing is, we ask for a bulk of
statistics and if one of them is missing an error is reported
instead of returning the rest. Since we ignore errors, we fail to
set statistics. Fix this by asking for each piece alone.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdevopenvswitch.c | 96 +++++++++++++++--------------------------
1 file changed, 34 insertions(+), 62 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 8f7215e06..6848f65b7 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -317,14 +317,8 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
{
virCommandPtr cmd = NULL;
char *output;
- long long rx_bytes;
- long long rx_packets;
- long long tx_bytes;
- long long tx_packets;
- long long rx_errs;
- long long rx_drop;
- long long tx_errs;
- long long tx_drop;
+ char *tmp;
+ bool gotStats = false;
int ret = -1;
/* Just ensure the interface exists in ovs */
@@ -340,67 +334,45 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
goto cleanup;
}
- VIR_FREE(output);
- virCommandFree(cmd);
-
- cmd = virCommandNew(OVSVSCTL);
- virNetDevOpenvswitchAddTimeout(cmd);
- virCommandAddArgList(cmd, "get", "Interface", ifname,
- "statistics:rx_bytes",
- "statistics:rx_packets",
- "statistics:tx_bytes",
- "statistics:tx_packets", NULL);
- virCommandSetOutputBuffer(cmd, &output);
-
- if (virCommandRun(cmd, NULL) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Interface doesn't have statistics"));
- goto cleanup;
- }
+#define GET_STAT(name, member) \
+ do { \
+ VIR_FREE(output); \
+ virCommandFree(cmd); \
+ cmd = virCommandNew(OVSVSCTL); \
+ virNetDevOpenvswitchAddTimeout(cmd); \
+ virCommandAddArgList(cmd, "get", "Interface", ifname, \
+ "statistics:" name, NULL); \
+ virCommandSetOutputBuffer(cmd, &output); \
+ if (virCommandRun(cmd, NULL) < 0) { \
+ stats->member = -1; \
+ } else { \
+ if (virStrToLong_ll(output, &tmp, 10, &stats->member) < 0 || \
+ *tmp != '\n') { \
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \
+ _("Fail to parse ovs-vsctl output")); \
+ goto cleanup; \
+ } \
+ gotStats = true; \
+ } \
+ } while (0)
/* The TX/RX fields appear to be swapped here
* because this is the host view. */
- if (sscanf(output, "%lld\n%lld\n%lld\n%lld\n",
- &tx_bytes, &tx_packets, &rx_bytes, &rx_packets) != 4) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Fail to parse ovs-vsctl output"));
- goto cleanup;
- }
+ GET_STAT("rx_bytes", tx_bytes);
+ GET_STAT("rx_packets", tx_packets);
+ GET_STAT("rx_errors", tx_errs);
+ GET_STAT("rx_dropped", tx_drop);
+ GET_STAT("tx_bytes", rx_bytes);
+ GET_STAT("tx_packets", rx_packets);
+ GET_STAT("tx_errors", rx_errs);
+ GET_STAT("tx_dropped", rx_drop);
- stats->rx_bytes = rx_bytes;
- stats->rx_packets = rx_packets;
- stats->tx_bytes = tx_bytes;
- stats->tx_packets = tx_packets;
-
- VIR_FREE(output);
- virCommandFree(cmd);
-
- cmd = virCommandNew(OVSVSCTL);
- virNetDevOpenvswitchAddTimeout(cmd);
- virCommandAddArgList(cmd, "get", "Interface", ifname,
- "statistics:rx_errors",
- "statistics:rx_dropped",
- "statistics:tx_errors",
- "statistics:tx_dropped", NULL);
- virCommandSetOutputBuffer(cmd, &output);
- if (virCommandRun(cmd, NULL) < 0) {
- /* This interface don't have errors or dropped, so set them to 0 */
- stats->rx_errs = 0;
- stats->rx_drop = 0;
- stats->tx_errs = 0;
- stats->tx_drop = 0;
- } else if (sscanf(output, "%lld\n%lld\n%lld\n%lld\n",
- &tx_errs, &tx_drop, &rx_errs, &rx_drop) == 4) {
- stats->rx_errs = rx_errs;
- stats->rx_drop = rx_drop;
- stats->tx_errs = tx_errs;
- stats->tx_drop = tx_drop;
- ret = 0;
- } else {
+ if (!gotStats) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Fail to parse ovs-vsctl output"));
+ _("Interface doesn't have statistics"));
goto cleanup;
}
+
ret = 0;
cleanup:
--
2.13.0
7 years, 9 months
[libvirt] RFC: Creating mediated devices with libvirt
by Erik Skultety
Hi all,
so there's been an off-list discussion about finally implementing creation of
mediated devices with libvirt and it's more than desired to get as many opinions
on that as possible, so please do share your ideas. This did come up already as
part of some older threads ([1] for example), so this will be a respin of the
discussions. Long story short, we decided to put device creation off and focus
on the introduction of the framework as such first and build upon that later,
i.e. now.
[1] https://www.redhat.com/archives/libvir-list/2017-February/msg00177.html
========================================
PART 1: NODEDEV-DRIVER
========================================
API-wise, device creation through the nodedev driver should be pretty
straightforward and without any issues, since virNodeDevCreateXML takes an XML
and does support flags. Looking at the current device XML:
<device>
<name>mdev_0cce8709_0640_46ef_bd14_962c7f73cc6f</name>
<path>/sys/devices/pci0000:00/.../0cce8709-0640-46ef-bd14-962c7f73cc6f</path>
<parent>pci_0000_03_00_0</parent>
<driver>
<name>vfio_mdev</name>
</driver>
<capability type='mdev'>
<type id='nvidia-11'/>
<iommuGroup number='13'/>
<uuid>UUID<uuid> <!-- optional enhancement, see below -->
</capability>
</device>
We can ignore <path>,<driver>,<iommugroup> elements, since these are useless
during creation. We also cannot use <name> since we don't support arbitrary
names and we also can't rely on users providing a name in correct form which we
would need to further parse in order to get the UUID.
So since the only thing missing to successfully use create an mdev using XML is
the UUID (if user doesn't want it to be generated automatically), how about
having a <uuid> subelement under <capability> just like PCIs have <domain> and
friends, USBs have <bus> & <device>, interfaces have <address> to uniquely
identify the device even if the name itself is unique.
Removal of a device should work as well, although we might want to
consider creating a *Flags version of the API.
=============================================================
PART 2: DOMAIN XML & DEVICE AUTO-CREATION, NO POLICY INVOLVED!
=============================================================
There were some doubts about auto-creation mentioned in [1], although they
weren't specified further. So hopefully, we'll get further in the discussion
this time.
>From my perspective there are two main reasons/benefits to that:
1) Convenience
For apps like virt-manager, user will want to add a host device transparently,
"hey libvirt, I want an mdev assigned to my VM, can you do that". Even for
higher management apps, like oVirt, even they might not care about the parent
device at all times and considering that they would need to enumerate the
parents, pick one, create the device XML and pass it to the nodedev driver, IMHO
it would actually be easier and faster to just do it directly through sysfs,
bypassing libvirt once again....
2) Future domain migration
Suppose now that the mdev backing physical devices support state dump and
reload. Chances are, that the corresponding mdev doesn't even exist or has a
different UUID on the destination, so libvirt would do its best to handle this
before the domain could be resumed.
Following what we already have:
<devices>
<hostdev mode='subsystem' type='mdev' model='vfio-pci'>
<source>
<address uuid='c2177883-f1bb-47f0-914d-32a22e3a8804'>
</source>
</hostdev>
</devices>
Instead of trying to somehow extend the <address> element using more
attributes like 'domain', 'slot', 'function', etc. that would render the whole
element ambiguous, I was thinking about creating a <parent> element nested under
<source> that would be basically just a nested definition of another host device
re-using all the element we already know, i.e. <address> for PCI, and of course
others if there happens to be a need for devices other than PCI. So speaking
about XML, we'd end up with something like:
<devices>
<hostdev mode='subsystem' type='mdev' model='vfio-pci'>
<source>
<parent>
<!-- possibly another <source> element - do we really want that? -->
<address domain='0x0000' bus='0x00' slot='0x00' function='0x00'>
<type id='foo'/>
<!-- end of potential <source> element -->
</parent>
<!-- this one takes precedence if it exists, ignoring the parent -->
<address uuid='c2177883-f1bb-47f0-914d-32a22e3a8804'>
</source>
</hostdev>
</devices>
So, this was the first idea off the top of my head, so I'd appreciate any
suggestions, comments, especially from people who have got the 'legacy'
insight into libvirt and can predict potential pitfalls based on experience :).
Thanks,
Erik
7 years, 9 months
[libvirt] [PATCH] util: Force reading of meta data to get encryption capacity value
by John Ferlan
https://bugzilla.redhat.com/show_bug.cgi?id=1371892
As it turns out the volume create, build, and refresh path was not peeking
at the meta data, so immediately after a create operation the value displayed
for capacity was still incorrect. However, if a pool refresh was done the
correct value was fetched as a result of a meta data peek.
The reason is it seems historically if the file type is RAW then peeking
at the file just took the physical value for the capacity. However, since
we know if it's an encrypted file, then peeking at the meta data will be
required in order to get a true capacity value.
So check for encryption in the source and if present, use the meta data
in order to fill in the capacity value and set the payload_offset.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virstoragefile.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index e82a7fb..11c3625 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3446,13 +3446,16 @@ virStorageSourceUpdateCapacity(virStorageSourcePtr src,
src->format = format;
}
- if (format == VIR_STORAGE_FILE_RAW)
+ if (format == VIR_STORAGE_FILE_RAW && !src->encryption) {
src->capacity = src->physical;
- else if ((meta = virStorageFileGetMetadataFromBuf(src->path, buf,
- len, format, NULL)))
+ } else if ((meta = virStorageFileGetMetadataFromBuf(src->path, buf,
+ len, format, NULL))) {
src->capacity = meta->capacity ? meta->capacity : src->physical;
- else
+ if (src->encryption && meta->encryption)
+ src->encryption->payload_offset = meta->encryption->payload_offset;
+ } else {
goto cleanup;
+ }
if (src->encryption && src->encryption->payload_offset != -1)
src->capacity -= src->encryption->payload_offset * 512;
--
2.9.4
7 years, 9 months
[libvirt] [PATCH v3 0/2] Adding locale support for virStrToDouble().
by Julio Faracco
The commits add locale support for virStrToDouble() due to differences between
the mantissa separator in different languages. For example, kernel always uses
dot to separate mantissa. An user who is using pt_BR locale (for example) uses
comma as a separator. So, this user will have problems to parse a kernel
settings using strtod() function.
One of commits move the virDoubleToStr() to virstring.* to share locale
global variables. Joining the two functions makes more sense.
Julio Faracco (2):
util: moving virDoubleToStr() from virutil to virstring.
util: fix locale problem with virStrToDouble().
src/util/virstring.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virstring.h | 3 ++
src/util/virutil.c | 63 ----------------------------------------
src/util/virutil.h | 3 --
4 files changed, 84 insertions(+), 66 deletions(-)
--
2.7.4
7 years, 9 months
[libvirt] libvirt-2.0.0 build error (hidden symbol libvirt_event_poll_update_handle_semaphore)
by longguang.yue
Hi, all:
i back port a patch which make qemu depends on util directory.
so i correct its dependency by applying a patch. but another error occur.
the patch is :
--- libvirt-2.0.0/src/Makefile.am 2016-06-27 22:12:20.523191076 +0800
+++ libvirt-2.0.0-ok/src/Makefile.am 2017-06-22 12:25:17.512000000 +0800
@@ -1362,6 +1362,7 @@
-I$(srcdir)/access \
-I$(srcdir)/conf \
-I$(srcdir)/secret \
+ -I$(srcdir)/util \
$(AM_CFLAGS)
libvirt_driver_qemu_impl_la_LDFLAGS = $(AM_LDFLAGS)
libvirt_driver_qemu_impl_la_LIBADD = $(CAPNG_LIBS) \
@@ -1369,6 +1370,7 @@
$(LIBNL_LIBS) \
$(LIBXML_LIBS) \
libvirt_secret.la \
+ libvirt_util.la \
$(NULL)
libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)
---------
BUILD ERROR:
CCLD qemucapsprobe
/bin/ld: .libs/qemucapsprobe: hidden symbol `libvirt_event_poll_update_handle_semaphore' in ../src/libvirt_probes.o is referenced by DSO
/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status
make[1]: *** [qemucapsprobe] Error 1
make[1]: Leaving directory `/root/libvirt-2.0/BUILD/libvirt-2.0.0/tests'
make: *** [check-am] Error 2
+ cat test-suite.log
cat: test-suite.log: No such file or directory
+ true
+ exit 1
error: Bad exit status from /var/tmp/rpm-tmp.ZVbGSv (%check)
RPM build errors:
Bad exit status from /var/tmp/rpm-tmp.ZVbGSv (%check)
-------------
7 years, 9 months
