netcf driver: use a single netcf handle for all connections

This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=983026

The netcf interface driver previously had no state driver associated
with it - as a connection was opened, it would create a new netcf
instance just for that connection, and close it when it was
finished. the problem with this is that each connection to libvirt
used up a netlink socket, and there is a per process maximum of ~1000
netlink sockets.

The solution is to create a state driver to go along with the netcf
driver. The state driver will opens a netcf instance, then all
connections share that same netcf instance, thus only a single
netlink socket will be used no matter how many connections are mde to
libvirtd.

This was rather simple to do - a new virObjectLockable class is
created for the single driverState object, which is created in
netcfStateInitialize and contains the single netcf handle; instead of
creating a new object for each client connection, netcfInterfaceOpen
now just increments the driverState object's reference count and puts
a pointer to it into the connection's privateData. Similarly,
netcfInterfaceClose() just un-refs the driverState object (as does
netcfStateCleanup()), and virNetcfInterfaceDriverStateDispose()
handles closing the netcf instance. Since all the functions already
have locking around them, the static lock functions used by all
functions just needed to be changed to call virObjectLock() and
virObjectUnlock() instead of directly calling the virMutex* functions.

diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c
index f47669e61bbeae8711d018147c534403cc2a45fc..fac059defb4739b099b255e7627e5baec3032341 100644 (file)
--- a/src/interface/interface_backend_netcf.c
+++ b/src/interface/interface_backend_netcf.c
@@ -41,21 +41,114 @@
 /* Main driver state */
 typedef struct
 {
-    virMutex lock;
+    virObjectLockable parent;
     struct netcf *netcf;
 } virNetcfDriverState, *virNetcfDriverStatePtr;
 
+static virClassPtr virNetcfDriverStateClass;
+static void virNetcfDriverStateDispose(void *obj);
 
-static void interfaceDriverLock(virNetcfDriverStatePtr driver)
+static int
+virNetcfDriverStateOnceInit(void)
+{
+    if (!(virNetcfDriverStateClass = virClassNew(virClassForObjectLockable(),
+                                       "virNetcfDriverState",
+                                       sizeof(virNetcfDriverState),
+                                       virNetcfDriverStateDispose)))
+        return -1;
+    return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virNetcfDriverState)
+
+static virNetcfDriverStatePtr driverState = NULL;
+
+
+static void
+virNetcfDriverStateDispose(void *obj)
 {
-    virMutexLock(&driver->lock);
+    virNetcfDriverStatePtr driver = obj;
+
+    if (driver->netcf)
+        ncf_close(driver->netcf);
+}
+
+
+static int
+netcfStateInitialize(bool privileged ATTRIBUTE_UNUSED,
+                     virStateInhibitCallback callback ATTRIBUTE_UNUSED,
+                     void *opaque ATTRIBUTE_UNUSED)
+{
+    if (virNetcfDriverStateInitialize() < 0)
+        return -1;
+
+    if (!(driverState = virObjectLockableNew(virNetcfDriverStateClass)))
+        return -1;
+
+    /* open netcf */
+    if (ncf_init(&driverState->netcf, NULL) != 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("failed to initialize netcf"));
+        virObjectUnref(driverState);
+        driverState = NULL;
+        return -1;
+    }
+    return 0;
 }
 
-static void interfaceDriverUnlock(virNetcfDriverStatePtr driver)
+
+static int
+netcfStateCleanup(void)
 {
-    virMutexUnlock(&driver->lock);
+    if (!driverState) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Attempt to close netcf state driver already closed"));
+        return -1;
+    }
+
+    if (virObjectUnref(driverState)) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Attempt to close netcf state driver "
+                         "with open connections"));
+        return -1;
+    }
+    driverState = NULL;
+    return 0;
 }
 
+
+static int
+netcfStateReload(void)
+{
+    int ret = -1;
+
+    if (!driverState)
+        return 0;
+
+    virObjectLock(driverState);
+    ncf_close(driverState->netcf);
+    if (ncf_init(&driverState->netcf, NULL) != 0)
+    {
+        /* this isn't a good situation, because we can't shut down the
+         * driver as there may still be connections to it. If we set
+         * the netcf handle to NULL, any subsequent calls to netcf
+         * will just fail rather than causing a crash. Not ideal, but
+         * livable (since this should never happen).
+         */
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("failed to re-init netcf"));
+        driverState->netcf = NULL;
+        goto cleanup;
+    }
+
+    ret = 0;
+cleanup:
+    virObjectUnlock(driverState);
+
+    return ret;
+}
+
+
 /*
  * Get a minimal virInterfaceDef containing enough metadata
  * for access control checks to be performed. Currently
@@ -148,61 +241,30 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct netcf *ncf, virInterfac
     return iface;
 }
 
-static virDrvOpenStatus netcfInterfaceOpen(virConnectPtr conn,
-                                           virConnectAuthPtr auth ATTRIBUTE_UNUSED,
-                                           unsigned int flags)
+static virDrvOpenStatus
+netcfInterfaceOpen(virConnectPtr conn,
+                   virConnectAuthPtr auth ATTRIBUTE_UNUSED,
+                   unsigned int flags)
 {
-    virNetcfDriverStatePtr driverState;
-
     virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
 
-    if (VIR_ALLOC(driverState) < 0)
-        goto alloc_error;
-
-    /* initialize non-0 stuff in driverState */
-    if (virMutexInit(&driverState->lock) < 0)
-    {
-        /* what error to report? */
-        goto mutex_error;
-    }
-
-    /* open netcf */
-    if (ncf_init(&driverState->netcf, NULL) != 0)
-    {
-        /* what error to report? */
-        goto netcf_error;
-    }
+    if (!driverState)
+        return VIR_DRV_OPEN_ERROR;
 
+    virObjectRef(driverState);
     conn->interfacePrivateData = driverState;
     return VIR_DRV_OPEN_SUCCESS;
-
-netcf_error:
-    if (driverState->netcf)
-    {
-        ncf_close(driverState->netcf);
-    }
-    virMutexDestroy(&driverState->lock);
-mutex_error:
-    VIR_FREE(driverState);
-alloc_error:
-    return VIR_DRV_OPEN_ERROR;
 }
 
-static int netcfInterfaceClose(virConnectPtr conn)
+static int
+netcfInterfaceClose(virConnectPtr conn)
 {
 
     if (conn->interfacePrivateData != NULL)
     {
-        virNetcfDriverStatePtr driver = conn->interfacePrivateData;
-
-        /* close netcf instance */
-        ncf_close(driver->netcf);
-        /* destroy lock */
-        virMutexDestroy(&driver->lock);
-        /* free driver state */
-        VIR_FREE(driver);
+        virObjectUnref(conn->interfacePrivateData);
+        conn->interfacePrivateData = NULL;
     }
-    conn->interfacePrivateData = NULL;
     return 0;
 }
 
@@ -408,11 +470,11 @@ static int netcfConnectNumOfInterfaces(virConnectPtr conn)
     if (virConnectNumOfInterfacesEnsureACL(conn) < 0)
         return -1;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
     count = netcfConnectNumOfInterfacesImpl(conn,
                                             NETCF_IFACE_ACTIVE,
                                             virConnectNumOfInterfacesCheckACL);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return count;
 }
 
@@ -424,12 +486,12 @@ static int netcfConnectListInterfaces(virConnectPtr conn, char **const names, in
     if (virConnectListInterfacesEnsureACL(conn) < 0)
         return -1;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
     count = netcfConnectListInterfacesImpl(conn,
                                            NETCF_IFACE_ACTIVE,
                                            names, nnames,
                                            virConnectListInterfacesCheckACL);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return count;
 
 }
@@ -442,11 +504,11 @@ static int netcfConnectNumOfDefinedInterfaces(virConnectPtr conn)
     if (virConnectNumOfDefinedInterfacesEnsureACL(conn) < 0)
         return -1;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
     count = netcfConnectNumOfInterfacesImpl(conn,
                                             NETCF_IFACE_INACTIVE,
                                             virConnectNumOfDefinedInterfacesCheckACL);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return count;
 }
 
@@ -458,12 +520,12 @@ static int netcfConnectListDefinedInterfaces(virConnectPtr conn, char **const na
     if (virConnectListDefinedInterfacesEnsureACL(conn) < 0)
         return -1;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
     count = netcfConnectListInterfacesImpl(conn,
                                            NETCF_IFACE_INACTIVE,
                                            names, nnames,
                                            virConnectListDefinedInterfacesCheckACL);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return count;
 
 }
@@ -490,7 +552,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
     if (virConnectListAllInterfacesEnsureACL(conn) < 0)
         return -1;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
 
     /* List all interfaces, in case of we might support new filter flags
      * except active|inactive in future.
@@ -621,7 +683,7 @@ cleanup:
         VIR_FREE(tmp_iface_objs);
     }
 
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -634,7 +696,7 @@ static virInterfacePtr netcfInterfaceLookupByName(virConnectPtr conn,
     virInterfacePtr ret = NULL;
     virInterfaceDefPtr def = NULL;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
     iface = ncf_lookup_by_name(driver->netcf, name);
     if (!iface) {
         const char *errmsg, *details;
@@ -662,7 +724,7 @@ static virInterfacePtr netcfInterfaceLookupByName(virConnectPtr conn,
 cleanup:
     ncf_if_free(iface);
     virInterfaceDefFree(def);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -675,7 +737,7 @@ static virInterfacePtr netcfInterfaceLookupByMACString(virConnectPtr conn,
     virInterfacePtr ret = NULL;
     virInterfaceDefPtr def = NULL;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
     niface = ncf_lookup_by_mac_string(driver->netcf, macstr, 1, &iface);
 
     if (niface < 0) {
@@ -711,7 +773,7 @@ static virInterfacePtr netcfInterfaceLookupByMACString(virConnectPtr conn,
 cleanup:
     ncf_if_free(iface);
     virInterfaceDefFree(def);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -726,7 +788,7 @@ static char *netcfInterfaceGetXMLDesc(virInterfacePtr ifinfo,
 
     virCheckFlags(VIR_INTERFACE_XML_INACTIVE, NULL);
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
 
     iface = interfaceDriverGetNetcfIF(driver->netcf, ifinfo);
     if (!iface) {
@@ -768,7 +830,7 @@ cleanup:
     ncf_if_free(iface);
     VIR_FREE(xmlstr);
     virInterfaceDefFree(ifacedef);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -784,7 +846,7 @@ static virInterfacePtr netcfInterfaceDefineXML(virConnectPtr conn,
 
     virCheckFlags(0, NULL);
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
 
     ifacedef = virInterfaceDefParseString(xml);
     if (!ifacedef) {
@@ -818,7 +880,7 @@ cleanup:
     ncf_if_free(iface);
     VIR_FREE(xmlstr);
     virInterfaceDefFree(ifacedef);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -828,7 +890,7 @@ static int netcfInterfaceUndefine(virInterfacePtr ifinfo) {
     virInterfaceDefPtr def = NULL;
     int ret = -1;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
 
     iface = interfaceDriverGetNetcfIF(driver->netcf, ifinfo);
     if (!iface) {
@@ -857,7 +919,7 @@ static int netcfInterfaceUndefine(virInterfacePtr ifinfo) {
 cleanup:
     ncf_if_free(iface);
     virInterfaceDefFree(def);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -871,7 +933,7 @@ static int netcfInterfaceCreate(virInterfacePtr ifinfo,
 
     virCheckFlags(0, -1);
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
 
     iface = interfaceDriverGetNetcfIF(driver->netcf, ifinfo);
     if (!iface) {
@@ -900,7 +962,7 @@ static int netcfInterfaceCreate(virInterfacePtr ifinfo,
 cleanup:
     ncf_if_free(iface);
     virInterfaceDefFree(def);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -914,7 +976,7 @@ static int netcfInterfaceDestroy(virInterfacePtr ifinfo,
 
     virCheckFlags(0, -1);
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
 
     iface = interfaceDriverGetNetcfIF(driver->netcf, ifinfo);
     if (!iface) {
@@ -943,7 +1005,7 @@ static int netcfInterfaceDestroy(virInterfacePtr ifinfo,
 cleanup:
     ncf_if_free(iface);
     virInterfaceDefFree(def);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -955,7 +1017,7 @@ static int netcfInterfaceIsActive(virInterfacePtr ifinfo)
     virInterfaceDefPtr def = NULL;
     int ret = -1;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
 
     iface = interfaceDriverGetNetcfIF(driver->netcf, ifinfo);
     if (!iface) {
@@ -985,7 +1047,7 @@ static int netcfInterfaceIsActive(virInterfacePtr ifinfo)
 cleanup:
     ncf_if_free(iface);
     virInterfaceDefFree(def);
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -1000,7 +1062,7 @@ static int netcfInterfaceChangeBegin(virConnectPtr conn, unsigned int flags)
     if (virInterfaceChangeBeginEnsureACL(conn) < 0)
         return -1;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
 
     ret = ncf_change_begin(driver->netcf, 0);
     if (ret < 0) {
@@ -1012,7 +1074,7 @@ static int netcfInterfaceChangeBegin(virConnectPtr conn, unsigned int flags)
                        details ? details : "");
     }
 
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -1026,7 +1088,7 @@ static int netcfInterfaceChangeCommit(virConnectPtr conn, unsigned int flags)
     if (virInterfaceChangeCommitEnsureACL(conn) < 0)
         return -1;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
 
     ret = ncf_change_commit(driver->netcf, 0);
     if (ret < 0) {
@@ -1038,7 +1100,7 @@ static int netcfInterfaceChangeCommit(virConnectPtr conn, unsigned int flags)
                        details ? details : "");
     }
 
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 
@@ -1052,7 +1114,7 @@ static int netcfInterfaceChangeRollback(virConnectPtr conn, unsigned int flags)
     if (virInterfaceChangeRollbackEnsureACL(conn) < 0)
         return -1;
 
-    interfaceDriverLock(driver);
+    virObjectLock(driver);
 
     ret = ncf_change_rollback(driver->netcf, 0);
     if (ret < 0) {
@@ -1064,13 +1126,13 @@ static int netcfInterfaceChangeRollback(virConnectPtr conn, unsigned int flags)
                        details ? details : "");
     }
 
-    interfaceDriverUnlock(driver);
+    virObjectUnlock(driver);
     return ret;
 }
 #endif /* HAVE_NETCF_TRANSACTIONS */
 
 static virInterfaceDriver interfaceDriver = {
-    "netcf",
+    .name = INTERFACE_DRIVER_NAME,
     .interfaceOpen = netcfInterfaceOpen, /* 0.7.0 */
     .interfaceClose = netcfInterfaceClose, /* 0.7.0 */
     .connectNumOfInterfaces = netcfConnectNumOfInterfaces, /* 0.7.0 */
@@ -1093,11 +1155,19 @@ static virInterfaceDriver interfaceDriver = {
 #endif /* HAVE_NETCF_TRANSACTIONS */
 };
 
+static virStateDriver interfaceStateDriver = {
+    .name = INTERFACE_DRIVER_NAME,
+    .stateInitialize = netcfStateInitialize,
+    .stateCleanup = netcfStateCleanup,
+    .stateReload = netcfStateReload,
+};
+
 int netcfIfaceRegister(void) {
     if (virRegisterInterfaceDriver(&interfaceDriver) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("failed to register netcf interface driver"));
         return -1;
     }
+    virRegisterStateDriver(&interfaceStateDriver);
     return 0;
 }