On 26/09/18 02:49, Spencer Olson wrote:
On Tue, Sep 25, 2018 at 6:26 AM Ian Abbott <abbotti@xxxxxxxxx> wrote:
I never noticed that you sort the data on module load. You also have an
exported function 'ni_sort_device_routes()' called internally by
'ni_sort_all_device_routes()' when the "ni_routes" module is loaded. Do
you envision that function ever being called externally?
I struggled with this one a tiny bit. The main reason why this is
exported is so that I could use it inside one of the unit tests in the
tests/ni_routes_test module (these were indeed invaluable for testing
on a system that didn't have real hardware attached). Otherwise, I do
not really imagine this being used externally. Since this function is
fully self-contained (i.e. only touches local variables and no module
variables), I figured that this export was at least safe.
That explains it, thanks.
I still think external linkage rather than .c file inclusion should be
doable. It shouldn't be that hard to link the "ni_routes" module from
several object files, and the consequent inability to use 'ARRAY_SIZE()'
on 'device_routes_list[]' and 'all_route_values[]' can be worked around
either by NULL terminating them or by storing their lengths in separate
variables.
If the likes of 'ni_660x_route_values' are declared with external
linkage, only "ni_routes" would be able to link to them if it is built
as a loadable module. If "ni_routes" is built into the kernel image
itself, then other code built into the kernel image would also be able
to link to them, but such code should be subject to review anyway.
In my working copy, I now have the ni_routing module that is comprised of:
ni_routes.o
ni_routing/ni_route_values.o
ni_routing/ni_route_values/ni_660x.o
ni_routing/ni_route_values/ni_eseries.o
ni_routing/ni_route_values/ni_mseries.o
ni_routing/ni_device_routes.o
(i.e. I've only broken out the components of what was originally
included in ni_routing/ni_route_values.c so far)
I am not very satisfied with the result--with respect to maintenance.
For the ni_route_values*, this is not too much of a deal since there
are not that many hardware families. For ni_device_routes*, this
seems a bit much, since each new hardware device implies
- a new compile unit in the Makefile ni_routing module
- two new lines in ni_routing/ni_device_routes.c (extern declaration
& addition to array)
- and a new file for the device route list in sub-directory ni_routing/
Currently, I have ni_route_values.c declaring each of the arrays from
ni_routing/ni_route_values/ni_*.o, rather than going overboard and
making .h files for these to include in ni_route_values.c. If I also
break out the arrays in ni_routing/ni_device_routes/*.c, I would make
this approach to avoid a fourth implication for maintenance. Breaking
out the ni_routing/ni_device_routes/*.c content into separate compile
units results in 18 more compile units that are linked into
ni_routing.ko and 18 additional lines in ni_device_routes.
I'm willing to be encouraged one way or another, but would like to see
your feedback/thoughts on the maintenance aspect.
You could perhaps lump all the external declarations of the individual
'struct ni_device_routes' and 'struct family_routes_values' variables
from the individual .c files into a couple of shared header files. It
doesn't matter if the header file declares external variables that are
not actually referenced by a particular compilation unit.
When adding a new .c file in ni_routing/ni_device_routes/ or
ni_routing/ni_route_values/, that would also need a single line addition
to a shared .h file, a single line addition to
ni_routing/ni_device_routes.c or ni_routing/ni_route_values.c, and a
change to list of objects in the Makefile used to build
ni_routing/ni_device_routes.o or ni_routing/ni_route_values.o.
--
-=( Ian Abbott <abbotti@xxxxxxxxx> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address: )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
