Hi Yifan,
On 5/6/26 12:17 AM, wuyifan wrote:
> Hi Reinette,
>
> On 4/23/2026 12:02 AM, Reinette Chatre wrote:
>> Hi Yifan,
>>
>> On 4/10/26 2:33 AM, Yifan Wu wrote:
>>> @@ -113,6 +115,7 @@ static int parse_imc_read_bw_events(char *imc_dir,
>>> unsigned int type,
>>> unsigned int *count)
>>> {
>>> char imc_events_dir[PATH_MAX], imc_counter_cfg[PATH_MAX];
>>> + struct imc_counter_config *imc_counter;
>>> unsigned int orig_count = *count;
>>> char cas_count_cfg[1024];
>>> struct dirent *ep;
>>> @@ -167,11 +170,17 @@ static int parse_imc_read_bw_events(char *imc_dir,
>>> unsigned int type,
>>> ksft_print_msg("Maximum iMC count exceeded\n");
>>> goto out_close;
>>> }
>>> + imc_counter = calloc(1, sizeof(*imc_counter));
>>> + if (!imc_counter) {
>>> + ksft_perror("Unable to allocate memory for iMC counters\n");
>>> + goto out_close;
>>> + }
>>> imc_counters_config[*count].type = type;
>>> get_read_event_and_umask(cas_count_cfg, *count);
>>> /* Do not fail after incrementing *count. */
>>> *count += 1;
>>> + list_add(&imc_counter->entry, &imc_counters_list);
>>> }
>>> if (*count == orig_count) {
>>> ksft_print_msg("Unable to find events in %s\n", imc_events_dir);
>> Should cleanup_read_mem_bw_imc() be called on error exit path?
> Thank you for your suggestion. When parse_imc_read_bw_events() exits with an
> error, the linked list imc_counters_list will be cleaned up in test_cleanup().
>
> main()
> └── run_single_test()
> ├── mbm_run_test()
> │ └── resctrl_val()
> │ └── mbm_init()
> │ └── initialize_read_mem_bw_imc()
> │ └── enumerate_imcs()
> │ └── read_from_imc_dir()
> │ └── parse_imc_read_bw_events()
> │ └── calloc()
> └── test_cleanup()
> └── mbm_test_cleanup()
> └── cleanup_read_mem_bw_imc()
>
> 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.
Reinette
