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

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(a)redhat.com&gt; [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(a)redhat.com&gt; >>> --- >>> 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.

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