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

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


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