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