Hi Reinette
On 5/8/2026 12:13 AM, Reinette Chatre wrote:
Calling cleanup_read_mem_bw_imc() in the error exit path may be intended
to prevent resource leaks. However, this results in the function being called
repeatedly in both the error exit branch and test_cleanup().
You are correct and calling it repeatedly is ok. When cleanup_read_mem_bw_imc()
is
called from test_cleanup() after a failure in parse_imc_read_bw_events() then it
will find that the list is empty and just be a no-op. This is safe.
Is there any specific intention behind calling it in parse_imc_read_bw_events()?
The motivation behind calling it in parse_imc_read_bw_events() is to not leave
this
memory allocated when this function fails. A function having a single
responsibility
is easier to use and maintain since a caller does not need to take into account
that
when the function fails it also needs to have additional responsibility to
clean up
the state left behind by it.
There may be some patterns where caller needs to clean up after a failure but
that is
usually done in an obvious way where the caller_immediately_ does the cleanup
on failure
but here this dependency is well hidden in this implementation with
test_cleanup() being
called so far from parse_imc_read_bw_events(). This hidden dependency makes
this code
difficult to use and maintain.
Or should the cleanup be uniformly handled in test_cleanup()?
Handling it only in test_cleanup() may work in current execution flow but if
the code is
ever re-factored this would result in a memory leak. It is not custom that
callers need
to clean up state when a function fails and since this allocation is buried
deep within the
execution flow I see this as a latent bug just waiting to be triggered.
Thank you for the detailed explanation. I understand the rationale now.
I will explicitly add cleanup calls in the error exit path in the next
version.
Best regards,
Yifan
