Repository navigation
[RFC] limiting visibility of internal structures #8100
Description
PROBLEM:
there are many structures in SOF that should be modified by dedicated functions only
BUT, unless the structure is protected somehow, it is extremely easy for anybody to access them directly. A developer may not even know that there are special API functions for reading/writing values,
Review is fine, but sooner or later somebody will, even accidently, violate the rule and make problems with further code flexibility
In C++ the issue is solved by compiler and public: / protected: / private: members of a class
In C, however, there's no perfect way to do it
The only way to make some of members private is to limit their range to single .c file (put a struct definition in .c) and export only proper API functions.
Unfortunately such API functions cannot be inline and therefore have significant performance impact
The purpose of this RFC is a brainstorm - how to do it to have a pretty good protection without loosing performance?
In example, FREE Rtos is doing this in a very hard way, having always 2 versions of structures: internal (in .c file) and exported (in .h), obfuscating all struct members names to meaningless dummy1, dummy2, etc.
i,e, exported:
typedef struct xSTATIC_TIMER
{
void * pvDummy1;
StaticListItem_t xDummy2;
TickType_t xDummy3;
void * pvDummy5;
TaskFunction_t pvDummy6;
#if ( configUSE_TRACE_FACILITY == 1 )
UBaseType_t uxDummy7;
#endif
uint8_t ucDummy8;
} StaticTimer_t;
internal
typedef struct tmrTimerControl /* The old naming convention is used to prevent breaking kernel aware debuggers. */
{
const char * pcTimerName; /**< Text name. This is not used by the kernel, it is included simply to make debugging easier. */ /*lint !e971 Unqualified char types are allowed for strings and single characters only. */
ListItem_t xTimerListItem; /**< Standard linked list item as used by all kernel features for event management. */
TickType_t xTimerPeriodInTicks; /**< How quickly and often the timer expires. */
void * pvTimerID; /**< An ID to identify the timer. This allows the timer to be identified when the same callback is used for multiple timers. */
portTIMER_CALLBACK_ATTRIBUTE TimerCallbackFunction_t pxCallbackFunction; /**< The function that will be called when the timer expires. */
#if ( configUSE_TRACE_FACILITY == 1 )
UBaseType_t uxTimerNumber; /**< An ID assigned by trace tools such as FreeRTOS+Trace */
#endif
uint8_t ucStatus; /**< Holds bits to say if the timer was statically allocated or not, and if it is active or not. */
} xTIMER;
This way is much to complicated in my opinion (yet really efficient)
MY PROPOSITION
if a member of a structure needs to be like C++ private, add a prefix, like PRIVATE or READONLY
i.e.
/** internals of sink API. NOT TO BE MODIFIED OUTSIDE OF sink_api_helper.h */
struct sof_sink {
const struct sink_ops *_READONLY_ops; /** operations interface */
size_t _PRIVATE_requested_write_frag_size; /** keeps number of bytes requested by get_buffer() */
size_t _PRIVATE_num_of_bytes_processed; /** processed bytes counter */
size_t _PRIVATE_obs; /** output buffer size as declared in module bind IPC */
struct sof_audio_stream_params *_PRIVATE_audio_stream_params; /** pointer to audio params */
};
I know it is not perfect nor pretty, yet if a developer sees a prefix, it will think twice before accessing the field directly
Any other ideas?
Activity
kv2019i commentedon Aug 28, 2023
I would be ok with the "PRIVATE" proposal, but I wonder if we could take an even simpler approach and just have the structs in the public header and have a comment only.
I think we (or at least some of us) were confused by two different classes of interfaces:
This is not super clean, but I think a pragmatic approach for performance sensitive code. If we can transfer the codebase in fairly limited time period, I think handling the policy via code reviews is doable. If we have mixed conventions in the codebase for longer periods of time (some code uses common accessors while rest uses fields directly), that will muddy the waters and review-only approach won't work.
marc-hb commentedon Aug 28, 2023
BTW: https://github.com/orgs/thesofproject/discussions, a bit more flexible and with threading.
This issue can be converted and transferred with one click on the right side.
lgirdwood commentedon Aug 28, 2023
We are currently bad at adding cheap get()/set() accessors and documenting our structures (with inline comments) for member scope and usage. i.e. if there is not a get()/set() then member should not be used, trusted, modified etc (and this needs to be spelled out in each header/structure).
The accessors can also handle any locking, cache, state machine and general correctness of structure usage. They are also easier for reviewers to check. e.g.
pipeline->periodvspipeline_get_period(pipeline)in client code.marcinszkudlinski commentedon Aug 29, 2023
discussion, 3 ideas:
_or double__for private membersin any case - add a clear comment to each private member
dbaluta commentedon Aug 29, 2023
@marcinszkudlinski I would go for option 1)
marc-hb commentedon Aug 30, 2023
BTW
kv2019i commentedon Aug 31, 2023
Also vote for option (1) and deploy this incrementally to new interfaces and see how well this works.
In Linux this has been successful just by enforced policy, so still a bit concerned whether we should just do better job at code review to catch inconsistent access to structs (when helpers exist).
mod: mark sink/src api as private in audio_stream
mod: mark sink/src api as private in audio_stream
mod: mark sink/src api as private in audio_stream
mod: mark sink/src api as private in audio_stream
kv2019i commentedon Dec 17, 2025
I think this is implemented now, closing.