[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 06/14] nodedev: Cleanup driver code and prototypes




On 05/26/2017 04:13 AM, Peter Krempa wrote:
> On Thu, May 25, 2017 at 15:57:03 -0400, John Ferlan wrote:
>> Alter the node_device_driver source and prototypes to follow more
>> recent code style guidelines w/r/t spacing between functions, format
>> of the function, and the prototype definitions.
>>
>> While the new names for nodeDeviceUpdateCaps, nodeDeviceUpdateDriverName,
>> and nodeDeviceGetTime don't follow exactly w/r/t a "vir" prefix, they
>> do follow other driver nomenclature style.
> 
> Still, this patch is mixing function renames with whitespace changes.
> 

So add "n" more patches - one to deal with whitespace... one to deal
with function renames... one to deal with argument placement...

I just tried to lump the formatting stuff into one - trying to avoid too
much minutiae.  The node_device* code is I would say the oldest and
least touched so it gets the most adjustment - too much.

>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>>  src/node_device/node_device_driver.c |  41 ++++--
>>  src/node_device/node_device_driver.h |  93 +++++++++----
>>  src/node_device/node_device_udev.c   | 256 +++++++++++++++++++++--------------
>>  3 files changed, 252 insertions(+), 138 deletions(-)
>>
>> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
>> index ba3da62..2a461fb 100644
>> --- a/src/node_device/node_device_driver.c
>> +++ b/src/node_device/node_device_driver.c
> 
>> @@ -151,11 +155,14 @@ void nodeDeviceLock(void)
>>  {
>>      virMutexLock(&driver->lock);
>>  }
>> +
>> +
>>  void nodeDeviceUnlock(void)
>>  {
>>      virMutexUnlock(&driver->lock);
>>  }
> 
> This one should be fixed too.
> 
> [...]

Oh right...

> 
>> @@ -478,8 +491,9 @@ nodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
>>      return ret;
>>  }
>>  
>> +
>>  static int
>> -get_time(time_t *t)
>> +nodeDeviceGetTime(time_t *t)
>>  {
>>      int ret = 0;
>>  
>> @@ -522,7 +536,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const char *wwpn)
> 
> Why didn't you change name of this function, since you changed the one
> above?
> 

Hmm... not sure why.  Strange - oh I see it already had the two space
between functions so I skipped right past it.

it's changed to nodeDeviceFindNewDevice in my branch now.

> [...]
> 
>> diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
>> index bc8af8a..b46f001 100644
>> --- a/src/node_device/node_device_driver.h
>> +++ b/src/node_device/node_device_driver.h
>> @@ -31,37 +31,75 @@
> 
> [...]
> 
>> +void
>> +nodeDeviceLock(void);
>> +
>> +void
>> +nodeDeviceUnlock(void);
>> +
>> +extern
>> +virNodeDeviceDriverStatePtr driver;
> 
> This is ugly. It's also not a function and 'extern' keyword is not the
> return type.

Oh right - this got to be such a rote exercise that I overdid it.  I
restored it back to one line.

> 
> [...]
> 
> ACK if you fix nodeDeviceUnlock too. Also please don't mix the header
> whitespace update with function renaming in future patches.
> 

Well the horse has already left the barn on that front for some on list
patches... But I can go back through those patches that haven't been
posted yet and create singular change.

Tks -

John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]