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@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@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.
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. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|