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

John Ferlan jferlan at redhat.com
Wed May 24 15:47:44 UTC 2017

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.


More information about the libvir-list mailing list