[libvirt] [PATCH 04/13] nodedev: Move node_device_linux_sysfs from node_driver to conf

[John Ferlan]

On 05/24/2017 10:35 AM, Daniel P. Berrange wrote:
> On Wed, May 24, 2017 at 09:45:09AM -0400, John Ferlan wrote:
>>
>>
>> On 05/23/2017 04:13 AM, Bjoern Walk wrote:
>>> John Ferlan <jferlan at redhat.com> [2017-05-19, 09:08AM -0400]:
>>>> Move the whole file from src/node_device into src/conf and rename the
>>>> API's to have the "virNodeDevice" prefix.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>>> ---
>>>> src/Makefile.am                                    |  8 ++++----
>>>> .../node_device_linux_sysfs.c                      | 24
>>>> ++++++++++------------
>>>> .../node_device_linux_sysfs.h                      |  9 +++++---
>>>> src/libvirt_private.syms                           |  5 +++++
>>>> src/node_device/node_device_driver.c               |  8 ++++----
>>>> src/node_device/node_device_hal.c                  |  4 ++--
>>>> src/node_device/node_device_udev.c                 |  4 ++--
>>>> 7 files changed, 34 insertions(+), 28 deletions(-)
>>>> rename src/{node_device => conf}/node_device_linux_sysfs.c (89%)
>>>> rename src/{node_device => conf}/node_device_linux_sysfs.h (82%)
>>>>
>>>
>>> What's the reasoning for this move? It somehow doesn't feel right and it
>>> will make backports harder.
>>
>> "Eventually" code that is currently in node_device_driver to peruse the
>> node device object list is going to move to virnodedevobj.c and there
>> was some "boundary" thing about calling back into the driver that I
>> don't quite recall the exact compile/link error which caused me to move
>> the code. Although the commit date is relatively recent - it's more or
>> less a copy of work done in Dec 2016.
> 
> This still doesn't make sense really.  The src/conf directory should
> only contain code that's related to the XML parsing/formatting, and
> generic object management.  It should never contain any functional
> driver code, and certainly none that pokes in sysfs.
> 

Other than the module function names having Sysfs in their names,
there's no sysfs poking in the code. There are calls to src/util
functions which do the sysfs poking. So I could change the function
names or just move the guts of the code into node_device_conf.c and then
call those APIs from the unmoved node_device_linux_sysfs.c. It was just
simpler to take this approach.

I can drop this and rework things.  It was created "backwards" from a
future need...

> 
>> One could also make the argument that there's nothing "driver specific"
>> in the code, so why should it have ever been put in the src/node_device
>> directory?  My other option would be to essentially move the guts of the
>> code into virnodedevobj and call it from src/node_device/...  IDC which
>> way this happens, but I just found this easier. Using gitk I've never
>> had issues in the past following history of the code when a module moves
>> from one directory to another.
>>
>> FWIW: My first instinct was move the code to src/util like other code
>> that has linux (or arch specific) pieces, but I couldn't do that because
>> node_device_linux_sysfs.h references "node_device_conf.h". Inclusion of
>> conf/*.h has been disallowed from src/util.
> 
> To get around that simply change the APIs so that instead of taking
> the virNodeDevXXX structs as a single parameter, you just pass in
> an exploded list of parameters, populated from the individual struct
> fields that are needed for the methods in question.
> 

IMO that's worse especially since _virNodeDevCapSCSIHost and
_virNodeDevCapPCIDev fields are getting filled in.

John