Re: [libvirt PATCH 3/6] nodedev: refactor nodeDeviceFindNewDevice()

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 4/30/20 11:42 PM, Jonathon Jongsma wrote:
> In preparation for creating mediated devices in libvirt, we will need to
> wait for new mediated devices to be created as well. Refactor
> nodeDeviceFindNewDevice() so that we can re-use the main logic from this
> function to wait for different device types by passing a different
> 'find' function.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
>   src/node_device/node_device_driver.c | 33 +++++++++++++++++++++++-----
>   1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 6a96a77d92..f948dfbf69 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -446,7 +446,8 @@ nodeDeviceGetTime(time_t *t)
>       return ret;
>   }
>   
> -
> +typedef virNodeDevicePtr (*nodeDeviceFindNewDeviceFunc)(virConnectPtr conn,
> +                                                        const void* opaque);

Again, some spaces here, and especially below.

>   /* When large numbers of devices are present on the host, it's
>    * possible for udev not to realize that it has work to do before we
>    * get here.  We thus keep trying to find the new device we just
> @@ -462,8 +463,8 @@ nodeDeviceGetTime(time_t *t)
>    */
>   static virNodeDevicePtr
>   nodeDeviceFindNewDevice(virConnectPtr conn,
> -                        const char *wwnn,
> -                        const char *wwpn)
> +                        nodeDeviceFindNewDeviceFunc func,
> +                        const void *opaque)
>   {
>       virNodeDevicePtr device = NULL;
>       time_t start = 0, now = 0;
> @@ -474,7 +475,7 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
>   
>           virWaitForDevices();
>   
> -        device = nodeDeviceLookupSCSIHostByWWN(conn, wwnn, wwpn, 0);
> +        device = func(conn, opaque);
>   
>           if (device != NULL)
>               break;
> @@ -487,6 +488,28 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
>       return device;
>   }
>   
> +typedef struct _NewSCISHostFuncData

s/SCIS/SCSI/

> +{
> +    const char *wwnn;
> +    const char *wwpn;
> +} NewSCSIHostFuncData;

We tend to like this split:

typedef struct _someStruct someStruct;
struct _someStruct {
  const char *member;
};

Honestly, I don't understand why, I guess it's just a coding style.

> +static virNodeDevicePtr
> +nodeDeviceFindNewSCSIHostFunc(virConnectPtr conn,
> +                              const void *opaque)
> +{
> +    const NewSCSIHostFuncData *data = opaque;
> +    return nodeDeviceLookupSCSIHostByWWN(conn, data->wwnn, data->wwpn, 0);
> +}
> +
> +static virNodeDevicePtr
> +nodeDeviceFindNewSCSIHost(virConnectPtr conn,
> +                          const char *wwnn,
> +                          const char *wwpn)
> +{
> +    NewSCSIHostFuncData data = { .wwnn = wwnn, .wwpn = wwpn};
> +    return nodeDeviceFindNewDevice(conn, nodeDeviceFindNewSCSIHostFunc, &data);
> +}
> +
>   static bool
>   nodeDeviceHasCapability(virNodeDeviceDefPtr def, virNodeDevCapType type)
>   {
> @@ -537,7 +560,7 @@ nodeDeviceCreateXML(virConnectPtr conn,
>           if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_CREATE) < 0)
>               return NULL;
>   
> -        device = nodeDeviceFindNewDevice(conn, wwnn, wwpn);
> +        device = nodeDeviceFindNewSCSIHost(conn, wwnn, wwpn);
>           /* We don't check the return value, because one way or another,
>            * we're returning what we get... */
>   

Michal