2:41 p.m.
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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>