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@xxxxxxxxxx> [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@xxxxxxxxxx>
>>>> ---
>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
