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

[John Ferlan]

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