[genivi-dlt] [patch] check payload length before casting
gunnar.x.andersson at volvocars.com
Mon Jan 23 12:39:37 EST 2017
Thanks for bringing the thread back to life. I have many open loops to track
> Hi Gunnar,
>> A) Is it necessary to check all input pointers, or only some? For
>> example, daemon_local - should we check it for NULL? Only some functions
>> check NULL for daemon_local pointer, which is confusing!
> I think all checks for daemon and daemon_local could be re-factored to use
> assert(). Both structures (are both really needed?) are for internal use
> and parameter to nearly all functions in daemon code, The daemon would just
> not work if these pointers are NULL.
Sounds good to me. Only remaining question is whether there ought to be
some "soft" way of failing in a production situation, if an assert fails
despite everything. Or is there is no point to that - you indicate it probably
needs to exit anyhow since it's not functional.
I guess systemd might restart a dlt-daemon if it exits, but how do
clients react to that? Can they recognize this and reconnect?
>> B) Who calls these functions? Since clients of DLT make use of library
>> functions I first imagined all of these calls are "internal" to DLT but
>> I'm not sure.
> All dlt_daemon_control_* are called when a DLT Client message is received.
> Depending on the message type, these functions are invoked by
> dlt_daemon_client_process_control which itself is called by
> dlt_daemon_process_client_message when data is received on a Client
> connection. Callstack would look like:
> -> dlt_daemon_process_client_message
> -> dlt_daemon_client_process_control
> -> dlt_daemon_control_*
> Inside dlt_daemon_process_client_message, data is read from corresponding
> connection, so it is ensured that data is available at this point.
> Therefore, also the pointer to msg could be handled with an assert.
Thanks for the clarification.
> To answer your question, all code under src/daemon is internal to dlt
> daemon and not called by someone else. But this raises the question if
> you want to trust all parts of the daemon or just inside same source
> file. For this, it might be necessary to check which functions should be
> static and which functions can be called from other parts of dlt-daemon.
> To take the example above, dlt_daemon_client_control is non-satic,
> whereas all dlt_demon_control_* function could be changed to be
All functions that are called only internally should be declared static of
course, so that a reader of the code can see this. There is no reason
to have those symbols exported. Why is that not already the case...
> And then we can trust dlt_daemon_client_control, because its directly
> under our control (and in the same source file). This also corresponds to
> your error handling category 1.
Yes, personally I usually consider all source files of the same component
to be in "my control" even if they call each other's functions across
source files (but only up to some size of project).
But as you indicate those functions are globally available in theory.
If only the functions are properly separated so that only the functions
that are supposed to be used externally are shown in particular header
files for external use, then it should be safe. A naming convention for
exported/non-exported functions also does not hurt.
> One could anyhow think about to make daemon, daemon_local pointers global
> to avoid having them as parameter to each and every function. In many
> cases they are just needed because another function which is called need
> access to either one or both of these pointers.
Yes, if they are so extremely central to the function it might make sense to
do so. What they refer to are in effect Singleton objects then?
Would that change be to reduce complexity and/or to improve performance?
Because you could even consider get_daemon() get_daemon_local() to get to the
pointers. Both ways, it saves a lot of passing variables around. With cross-module
inlining the calls can probably also be inlined (if performance is a concern).
> Best regards
More information about the genivi-diagnostic-log-and-trace