On Thu, Jul 28, 2022 at 03:17:08PM +0100, Carsten Haitzler wrote:
[...]
- }
- /* Realloc is good enough, though we could realloc by chunks, not that
* anyone will ever measure performance here */
- files_tmp = realloc(files,
(files_num_tmp + 1) * sizeof(struct script_file));
files = realloc(files, (files_num_tmp + 1) * sizeof(struct script_file));
BTW, I don't see any where to free the memory for "files".
The very first comment at the top of the code (after headers) that you wanted the format of changed explains this right above the static vars. There is no point. This is code for a utility that runs tests and runs them once ever and then exits. There is no need for a cleanup/free as it's all freed on process exit. It's a singleton design pattern so it scans for these test files once ever for the whole run of the tests (unlike before it'd scan multiple times with all the I/O overhead and syscall costs).
Okay, I understand your meaning. But I still think that it's not a good practice if program doesn't explictly release memory.
I will review new patch and check if we can release the buffer in somewhere in the code.
- if (files_tmp == NULL) {
pr_err("Out of memory while building test list\n");
abort();
- }
- /* Add file to end and NULL terminate the struct array */
- files = files_tmp;
- files_num = files_num_tmp;
- files[files_num - 1].dir = strdup_check(dir);
- files[files_num - 1].file = strdup_check(file);
- files[files_num - 1].desc = strdup_check(desc);
- files[files_num].dir = NULL;
- files[files_num].file = NULL;
- files[files_num].desc = NULL;
I personally think here it's over complex for using the last item in the array as a redundant item and set NULL to its fields. We have the global variable "files_num", which can be used for checking boundary for the array.
See how the code is called. It makes the calling simpler as you don't need to get both the array and the num/count. You just get the array and iterate until a NULL item. So the calling convention is simplified. That's why I did it. It's a very common design pattern that saves some complexity at the caller end with the cost of a "null item" overhead. Just do ptr++ as you iterate over the files until a null item - much simpler calling code.
Okay, this would be fine for me.
[...]
I strongly suggest you to consider how to organize the patches with better format. This patch actually finishes below things:
- Support sub folder searching for shell script (so the key change is using the recursion in append_scripts_in_dir());
- Refactoring to a common code for iterating testing scripts;
- Many minor refactoring, like dropping macro for_each_shell_test() and introduces new function is_shell_script().
Seems every part above is deserved to use a separate patch, which would be much easier for review.
It was not written in stages at all but wholesale written as a new file to replace the functionality in the previous file that was duplicated multiple times (scandir). The new code could also recurse too. I'd have to bend over backwards to create an artificial new set of code that uses all the old code then iterate it multiple times to do this. It's not how it happened.
It should be drastically easier to review this way as you can easily just see the old code vanish and how it now calls the new code (and how simple it is) and then the whole new code beng called - review it as new code. :)
Just remind, at the end you need to convince maintainers for the patches, this is why I suggest you to use small pieces of patches rather than a single big patch. :-)
My personal experience is small chunk patch is much easier for review. Anyway, I will review the new patch set.
Thanks, Leo