)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1000853,"name":"zapb","display_name":"Marc Schink","email":"dev@zapb.de","username":"zapb"},"change_message_id":"90f415841d32efd4b57cde2489da39c7f7844526","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"7d88df89_6dda70b4","updated":"2024-12-10 13:16:42.000000000","message":"There is a typo in your commit message \u0027itialisation\u0027 -\u003e \u0027initialisation\u0027","commit_id":"6749fe72a485ebfe749682be6f9afd603f794d22"},{"author":{"_account_id":1000853,"name":"zapb","display_name":"Marc Schink","email":"dev@zapb.de","username":"zapb"},"change_message_id":"6f2e110fd942b20c064e5d98c616ad427d208559","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"6780389a_ea872ddb","updated":"2024-12-10 13:15:48.000000000","message":"https://review.openocd.org/c/openocd/+/8637 (for reference)","commit_id":"6749fe72a485ebfe749682be6f9afd603f794d22"},{"author":{"_account_id":1002047,"name":"Evgeniy Naydanov","email":"eugnay@gmail.com","username":"en-sc"},"change_message_id":"2e7a979615df9560639ff5c9194ea9b476f92338","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"a74afb59_468582c4","updated":"2024-12-10 19:17:13.000000000","message":"LGTM.\n\nHowever, I\u0027m a bit concerned with the fact `free()` is called directly. What if there is a dynamically allocated field in `target-\u003eprivate_config`? I believe Marc raised similar concerns in 8637.\n\nSome time ago I\u0027ve tried to move around the call to `target-\u003etype-\u003ecreate` to before the call to `target-\u003etype-\u003etarget_jim_configure` in https://review.openocd.org/c/openocd/+/8434 .\nThe motivation for that patch was -- I didn\u0027t know about the `private_config` field and tried to store target-type-specific configuration in `target-\u003earch_info` :).\n\nAs Antonio [mentioned there](https://review.openocd.org/c/openocd/+/8434/comments/6861aba0_e4171470) it should be possible to merge `private_config` into `arch_info` but he would like to merge the transition away from `jim_handler` first.","commit_id":"ed634ce1387b197126bfbc7fbd4bc5f9ccced7ca"},{"author":{"_account_id":1002047,"name":"Evgeniy Naydanov","email":"eugnay@gmail.com","username":"en-sc"},"change_message_id":"7ef44e941f5c6e1257790595529cc228106037d4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"0e07123a_da1795ca","in_reply_to":"044702f5_c4214d48","updated":"2024-12-11 09:44:56.000000000","message":"\u003e Could you please explain what do you mean by \"a dynamically allocated field\" ...\n\nE.g. something like:\n```\nstruct some_target_private_config {\n    my_type *some_opt_value;\n    ....\n}\n```\n\nAnd configuring some option results in `((some_target_private_config *)target-\u003eprivate_config)-\u003esome_opt_value \u003d malloc(...);`.\n\nThis used to be perfectly valid before the change, since `private_config` used to be deallocated only by a call to `target-\u003etype-\u003edeinit_target`, which knows about the peculiarities of a particular target type and will call `free((some_target_private_config *)target-\u003eprivate_config)-\u003esome_opt_value)` before calling `free(target-\u003eprivate_config)`.\n\nHowever, this is only a hypothetical and is not an issue with the current codebase.","commit_id":"ed634ce1387b197126bfbc7fbd4bc5f9ccced7ca"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"4321f86bf7eb886d5a4a77ff6435d0669eb07d05","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"3462109b_1649c3d6","in_reply_to":"0e07123a_da1795ca","updated":"2024-12-11 10:45:00.000000000","message":"I see, good point.\nIn such case we should make `private_config` to be a full featured object - witch at least configure a free methods","commit_id":"ed634ce1387b197126bfbc7fbd4bc5f9ccced7ca"},{"author":{"_account_id":1000853,"name":"zapb","display_name":"Marc Schink","email":"dev@zapb.de","username":"zapb"},"change_message_id":"114397597dd1248a6028736fb0e65582df0e60e7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"70ce2c42_5d25acdc","in_reply_to":"3462109b_1649c3d6","updated":"2024-12-12 12:39:22.000000000","message":"Since `private_config` is always populated by target-specific code, it should also be cleared by the same \u0027layer\u0027 in my opinion. In contrast to aarch64, the allocation is not that obvious for cortex_{a,m} because `target_jim_configure` is directly \"outsourced\" to the adiv5 code. I would propose to always call `xxx_deinit_target()` - also during the configuration stage.\nAlso, I think we should implement a \"free\" function for adiv5_private_config and not call `free()` directly on it. This function is then call in the `xxx_deinit_target()` code.\n\nAny thoughts / objections?\n\n@Tomas do you want to work on a \"more general solution\" or should we push this patch as is?","commit_id":"ed634ce1387b197126bfbc7fbd4bc5f9ccced7ca"},{"author":{"_account_id":1000853,"name":"zapb","display_name":"Marc Schink","email":"dev@zapb.de","username":"zapb"},"change_message_id":"8533c9d10b25c80a3efdd0dc9b43192bf1340f21","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"79e97043_026736aa","in_reply_to":"3b4c864b_6efbbc59","updated":"2024-12-12 17:53:49.000000000","message":"Yep, let\u0027s merge this patch and work on more general solution later.\n\nYes, there will be segfaults due to invalid reads etc. Using `xxx_deinit_target()` needs good testing.","commit_id":"ed634ce1387b197126bfbc7fbd4bc5f9ccced7ca"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"6ba15014a2cb7ccc17df9d1a066d3fe8fcfc7250","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"3b4c864b_6efbbc59","in_reply_to":"70ce2c42_5d25acdc","updated":"2024-12-12 13:44:07.000000000","message":"Yet it makes sense.\nUnfortunately to do this we should first carefully check all targets if calling `xxx_deinit_target()` without previous success of `xxx_target_create()` is safe.\nI suspect doing so could result in dereference of NULL pointers or something similar in some obscure/rare used targets. And keep in mind that nobody is able to test all targets code. Am I too sceptic?\n\nLet\u0027s take this patch as the fast fix with minimal changes to the existing code. Merging it will not prevent you or anybody else to propose a \"more general\" or better solution.","commit_id":"ed634ce1387b197126bfbc7fbd4bc5f9ccced7ca"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"6396f905891ef1f953320c4e7f8c4c91309e4b9c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"044702f5_c4214d48","in_reply_to":"a74afb59_468582c4","updated":"2024-12-11 02:52:50.000000000","message":"\u003e However, I\u0027m a bit concerned with the fact `free()` is called directly. What if there is a dynamically allocated field in `target-\u003eprivate_config`?\n\nCould you please explain what do you mean by \"a dynamically allocated field\" and what potential problem you see? AFAIK there is always either NULL or result of calloc in `target-\u003eprivate_config`","commit_id":"ed634ce1387b197126bfbc7fbd4bc5f9ccced7ca"}]}