1. Patchew
  2. Libvirt
  3. [libvirt PATCH 0/6] Add ability to create mediated devices in libvirt
  4. View patch

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

Jonathon Jongsma posted 6 patches 5 years, 9 months ago
There is a newer version of this series
[libvirt PATCH 3/6] nodedev: refactor nodeDeviceFindNewDevice()
Posted by Jonathon Jongsma 5 years, 9 months ago 
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@redhat.com>
---
 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);
 /* 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
+{
+    const char *wwnn;
+    const char *wwpn;
+} NewSCSIHostFuncData;
+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... */
 
-- 
2.21.1
Re: [libvirt PATCH 3/6] nodedev: refactor nodeDeviceFindNewDevice()
Posted by Michal Privoznik 5 years, 9 months ago 
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@redhat.com>
> ---
>   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
Re: [libvirt PATCH 3/6] nodedev: refactor nodeDeviceFindNewDevice()
Posted by Erik Skultety 5 years, 8 months ago 
On Mon, May 11, 2020 at 03:27:58PM +0200, Michal Privoznik wrote:
> 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@redhat.com>
> > ---
> >   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.

The typedef doesn't really bring any benefit in this case. I don't have any
tangible statistics for this, just a plain git grep, but it looks like that the
prevailing style for such private structures actually *is* without any
typedefing which also makes sense.

No further comments on this patch from my side.

-- 
Erik Skultety

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.