[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/3] nodedev: Add driver callback mechanism to add/remove devices



John Ferlan <jferlan redhat com> [2017-02-09, 04:36AM +0100]:
Add a callback mechanism as a side door of sorts to the event
mgmt functions that are triggered when a node_device object is
added or removed. This includes a mechanism to enumerate already
added devices for those stateInitialize functions called after
the initial nodedevRegister is called.

Signed-off-by: John Ferlan <jferlan redhat com>
---
src/check-aclrules.pl              |   3 +-
src/check-driverimpls.pl           |   1 +
src/conf/node_device_conf.c        | 125 ++++++++++++++++++++++++++++++++++++-
src/conf/node_device_conf.h        |  18 ++++++
src/libvirt_private.syms           |   3 +
src/node_device/node_device_udev.c |  29 +++++++++
6 files changed, 177 insertions(+), 2 deletions(-)

diff --git a/src/check-aclrules.pl b/src/check-aclrules.pl
index 8739cda..161d954 100755
--- a/src/check-aclrules.pl
+++ b/src/check-aclrules.pl
@@ -243,7 +243,8 @@ while (<>) {
    } elsif (/^(?:static\s+)?(vir(?:\w+)?Driver)\s+/) {
        if ($1 ne "virNWFilterCallbackDriver" &&
            $1 ne "virNWFilterTechDriver" &&
-            $1 ne "virDomainConfNWFilterDriver") {
+            $1 ne "virDomainConfNWFilterDriver" &&
+            $1 ne "virNodeDeviceCallbackDriver") {
            $intable = 1;
            $table = $1;
        }
diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl
index e320558..b707fc8 100755
--- a/src/check-driverimpls.pl
+++ b/src/check-driverimpls.pl
@@ -70,6 +70,7 @@ while (<>) {
    } elsif (/^(?:static\s+)?(vir(?:\w+)?Driver)\s+/) {
        next if $1 eq "virNWFilterCallbackDriver" ||
                $1 eq "virNWFilterTechDriver" ||
+                $1 eq "virNodeDeviceCallbackDriver" ||
                $1 eq "virConnectDriver";
        $intable = 1;
        $table = $1;
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 4d3268f..7a4df44 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -33,6 +33,7 @@
#include "virstring.h"
#include "node_device_conf.h"
#include "device_conf.h"
+#include "virlog.h"
#include "virxml.h"
#include "virbuffer.h"
#include "viruuid.h"
@@ -40,6 +41,8 @@

#define VIR_FROM_THIS VIR_FROM_NODEDEV

+VIR_LOG_INIT("conf.node_device_conf");
+
VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST,
              "system",
              "pci",
@@ -263,6 +266,104 @@ void virNodeDeviceDefFree(virNodeDeviceDefPtr def)
    VIR_FREE(def);
}

+
+/* Provide callback infrastructure that is expected to be called when
+ * devices are added/removed from a node_device subsystem that supports
+ * the callback mechanism. This provides a way for drivers to register
+ * to be notified when a node_device is added/removed. */
+virNodedevEnumerateAddDevices virNodedevEnumerateAddDevicesCb = NULL;
+int nodeDeviceCallbackDriver;
+#define MAX_CALLBACK_DRIVER 10
+static virNodeDeviceCallbackDriverPtr nodeDeviceDrvArray[MAX_CALLBACK_DRIVER];
+
+
+/**
+ * @cb: Driver function to be called.
+ *
+ * Set a callback at driver initialization to provide a callback to a
+ * driver specific function that can handle the enumeration of the existing
+ * devices and the addition of those devices for the registering driver.
+ *
+ * The initial node_device enumeration is done prior to other drivers, thus
+ * this provides a mechanism to load the existing data since the functions
+ * virNodeDeviceAssignDef and virNodeDeviceObjRemove would typically
+ * only be called when a new device is added/removed after the initial
+ * enumeration. The registering driver will need to handle duplication
+ * of data.
+ */
+void
+virNodeDeviceConfEnumerateInit(virNodedevEnumerateAddDevices cb)
+{
+    virNodedevEnumerateAddDevicesCb = cb;
+}
+
+
+/**
+ * @cbd: Driver callback pointers to add/remove devices
+ *
+ * Register a callback function in the registering driver to be called
+ * when devices are added or removed. Additionally, ensure the initial
+ * enumeration of the devices is completed taking care to do it after
+ * setting the callbacks
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int
+virNodeDeviceRegisterCallbackDriver(virNodeDeviceCallbackDriverPtr cbd)
+{
+    if (nodeDeviceCallbackDriver >= MAX_CALLBACK_DRIVER) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("no space available for another callback driver"));
+        return -1;
+    }
+
+    if (!cbd->nodeDeviceAdd || !cbd->nodeDeviceRemove) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("callback requires both add and remove functions"));
+        return -1;
+    }
+
+    nodeDeviceDrvArray[nodeDeviceCallbackDriver++] = cbd;
+
+    /* Setting the add/remove callback first ensures that there is no
+     * window of opportunity for a device to be added after enumeration
+     * is complete, but before the callback is in place. So, set the
+     * callback first, then do the enumeration. */
+    if (virNodedevEnumerateAddDevicesCb) {
+        if (virNodedevEnumerateAddDevicesCb(cbd->nodeDeviceAdd) < 0) {
+            nodeDeviceDrvArray[--nodeDeviceCallbackDriver] = NULL;
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+
+/**
+ * @cb: Driver function to be called.
+ *
+ * Clear the callback function. It'll be up to the calling driver to
+ * clear it's own data properly
+ */
+void
+virNodeDeviceUnregisterCallbackDriver(virNodeDeviceCallbackDriverPtr cbd)
+{
+    size_t i = 0;
+
+    while (i < nodeDeviceCallbackDriver && nodeDeviceDrvArray[i] != cbd)
+        i++;
+
+    if (i < nodeDeviceCallbackDriver) {
+        memmove(&nodeDeviceDrvArray[i], &nodeDeviceDrvArray[i+1],
+                (nodeDeviceCallbackDriver - i - 1) *
+                sizeof(nodeDeviceDrvArray[i]));
+        nodeDeviceDrvArray[i] = NULL;
+        nodeDeviceCallbackDriver--;
+    }
+}
+
+
void virNodeDeviceObjFree(virNodeDeviceObjPtr dev)
{
    if (!dev)
@@ -289,6 +390,7 @@ void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs)
virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs,
                                           virNodeDeviceDefPtr def)
{
+    size_t i;
    virNodeDeviceObjPtr device;

    if ((device = virNodeDeviceFindByName(devs, def->name))) {
@@ -315,6 +417,18 @@ virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs,
    }
    device->def = def;

+    /* Call any registered drivers that want to be notified of a new device */
+    for (i = 0; i < nodeDeviceCallbackDriver; i++) {
+        if (nodeDeviceDrvArray[i]->nodeDeviceAdd(def, false) < 0) {
+            VIR_DELETE_ELEMENT(devs->objs, devs->count - 1, devs->count);

I don't understand the reasoning why a failure to process the device on
one listening driver leads to the complete rejection of the device in
the node device driver.

+            virNodeDeviceObjUnlock(device);
+            virNodeDeviceObjFree(device);
+            /* NB: If we fail to remove from one driver - it's not a problem
+             * since adding a new device wouldn't fail if already found */
+            return NULL;
+        }
+    }
+
    return device;

}
@@ -322,13 +436,22 @@ virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs,
void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
                            virNodeDeviceObjPtr *dev)
{
-    size_t i;
+    size_t i, j;

    virNodeDeviceObjUnlock(*dev);

    for (i = 0; i < devs->count; i++) {
        virNodeDeviceObjLock(*dev);
        if (devs->objs[i] == *dev) {
+            /* Call any registered drivers that want to be notified of a
+             * removed device */
+            for (j = 0; j < nodeDeviceCallbackDriver; j++) {
+                if (nodeDeviceDrvArray[j]->nodeDeviceRemove((*dev)->def) < 0) {
+                    VIR_DEBUG("failed to remove name='%s' parent='%s' from "
+                              "callback driver, continuing",
+                              (*dev)->def->name, (*dev)->def->parent);
+                }
+            }
            virNodeDeviceObjUnlock(*dev);
            virNodeDeviceObjFree(devs->objs[i]);
            *dev = NULL;
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 1634483..925cb6e 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -207,6 +207,24 @@ struct _virNodeDeviceDef {
    virNodeDevCapsDefPtr caps;		/* optional device capabilities */
};

+/* Callback mechanism to add/remove node device's by name/parent
+ * to a target driver that cares to know */
+typedef int (*virNodeDeviceAdd)(virNodeDeviceDefPtr def, bool enumerate);
+typedef int (*virNodeDeviceRemove)(virNodeDeviceDefPtr def);
+
+typedef struct _virNodeDeviceCallbackDriver virNodeDeviceCallbackDriver;
+typedef virNodeDeviceCallbackDriver *virNodeDeviceCallbackDriverPtr;
+struct _virNodeDeviceCallbackDriver {
+    const char *name;
+    virNodeDeviceAdd nodeDeviceAdd;
+    virNodeDeviceRemove nodeDeviceRemove;
+};
+
+typedef int (*virNodedevEnumerateAddDevices)(virNodeDeviceAdd deviceAddCb);
+void virNodeDeviceConfEnumerateInit(virNodedevEnumerateAddDevices cb);
+
+int virNodeDeviceRegisterCallbackDriver(virNodeDeviceCallbackDriverPtr);
+void virNodeDeviceUnregisterCallbackDriver(virNodeDeviceCallbackDriverPtr);

typedef struct _virNodeDeviceObj virNodeDeviceObj;
typedef virNodeDeviceObj *virNodeDeviceObjPtr;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d556c7d..6d11463 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -695,6 +695,7 @@ virNodeDevCapsDefFree;
virNodeDevCapTypeFromString;
virNodeDevCapTypeToString;
virNodeDeviceAssignDef;
+virNodeDeviceConfEnumerateInit;
virNodeDeviceDefFormat;
virNodeDeviceDefFree;
virNodeDeviceDefParseFile;
@@ -713,6 +714,8 @@ virNodeDeviceObjListFree;
virNodeDeviceObjLock;
virNodeDeviceObjRemove;
virNodeDeviceObjUnlock;
+virNodeDeviceRegisterCallbackDriver;
+virNodeDeviceUnregisterCallbackDriver;


# conf/node_device_event.h
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 4b81312..ea6970b 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1457,6 +1457,33 @@ static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED)
    return 0;
}

+
+/*
+ * @deviceAddCb: Callback routine for adding a device
+ *
+ * Enumerate all known devices calling the add device callback function
+ *
+ * Returns 0 on success, -1 on failure
+ */
+static int
+udevEnumerateAddDevices(virNodeDeviceAdd deviceAddCb)
+{
+    size_t i;
+    int ret = 0;
+
+    nodeDeviceLock();
+    for (i = 0; i < driver->devs.count && ret >= 0; i++) {
+        virNodeDeviceObjPtr obj = driver->devs.objs[i];
+        virNodeDeviceObjLock(obj);
+        ret = deviceAddCb(obj->def, true);
+        virNodeDeviceObjUnlock(obj);
+    }
+    nodeDeviceUnlock();
+
+    return ret;
+}
+
+
static int nodeStateInitialize(bool privileged,
                               virStateInhibitCallback callback ATTRIBUTE_UNUSED,
                               void *opaque ATTRIBUTE_UNUSED)
@@ -1535,6 +1562,8 @@ static int nodeStateInitialize(bool privileged,
    if (udevEnumerateDevices(udev) != 0)
        goto cleanup;

+    virNodeDeviceConfEnumerateInit(udevEnumerateAddDevices);

I don't quite see the need for this callback indirection. What other
drivers want to implement a different enumeration method for devices?

+
    ret = 0;

 cleanup:
--
2.7.4

--
libvir-list mailing list
libvir-list redhat com
https://www.redhat.com/mailman/listinfo/libvir-list


--
IBM Systems
Linux on z Systems & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland
Schönaicher Str. 220
71032 Böblingen
Phone: +49 7031 16 1819
E-Mail: bwalk de ibm com
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]