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