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);
+            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);
+
     ret = 0;
 
  cleanup:
-- 
2.7.4

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

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 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 02/09/2017 07:52 AM, Bjoern Walk wrote:

[...]

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

That's why there's a code review ;-)  If I write this without a failure,
someone could query why I chose that!

One can consider this like an event - failing to fail means the event
isn't delivered. If there are nodedev types that rely on one another
that could be a problem.

For example, a scsi_target relies on a scsi_host being present. If we
skip/ignore the scsi_host event, then the subsequent scsi_target event
will fail to find the scsi_host and fail as well.

So how does one "fix" that? That is how to "re"-enumerate. In any case,
at least a way to message a failure via VIR_WARN I think would be
necessary. I'm fine with not failing out.

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

[...]

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

I struggled with a couple of different mechanisms in order to enumerate
the "existing" node devices as the node devices are discovered/added in
virNodeDeviceAssignDef before qemu is started.

The one thing I really needed was a way to traverse the node device
objects in order to grab/pass the node device def since vHBA creation
would need to know the wwnn/wwpn to search/match a domain definition
with the same wwnn/wwpn.

So one option was to call the virConnectListAllNodeDevices in order to
get a list of all devices by name (and well parent if the other patch is
ACK'd and pushed). Having the name/parent wasn't quite enough as I
needed to get the virNodeDeviceDefPtr (because I'd need the wwnn/wwpn).
There is a virNodeDeviceGetWWNs API, but it requires a node device ptr.

Another option would be to add an API that would take a name and perform
the wwnn/wwpn fetch based on that. Similar to the virConnect API - that
would require having a connection pointer.

So I chose this methodology which allowed the udev driver to provide the
enumeration API that virNodeDeviceRegisterCallbackDriver would be able
to call.  But of course I see your point (now) - how would a different
driver registration be able to delineate which cb API to call. Of course
it solved *my* problem, but isn't quite generic enough I guess <sigh>.

Going to need to think of a different mechanism I guess... Drat.  Any
suggestions? ;-)


John
>> +
>>     ret = 0;
>>
>>  cleanup:
>> -- 
>> 2.7.4
>>
>> -- 
>> 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