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

[Erik Skultety]

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 at 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