From 6b1a948213c7db3eede8350f534ec3813e8b296e Mon Sep 17 00:00:00 2001

Message-Id: <6b1a948213c7db3eede8350f534ec3813e8b296e.1379193141.git.jdenemar@redhat.com>

From: Laine Stump <laine@laine.org>

Date: Thu, 12 Sep 2013 06:32:06 -0600

Subject: [PATCH] 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.

(cherry picked from commit 822fe1367dd55e3de7c3c044ee02fd733ab3415a)

---

 src/interface/interface_backend_netcf.c | 230 +++++++++++++++++++++-----------

 1 file changed, 150 insertions(+), 80 deletions(-)

diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c

index f47669e..fac059d 100644

--- 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;

 }

-- 

1.8.3.2