Re: [libvirt PATCH 0/9] eliminate (almost) all VIR_FREE in esx directory
On 2/12/21 11:07 PM, Laine Stump wrote: 142 more down, 3084 to go :-) This goes through all the standard methods of eliminating VIR_FREE - first converting as many pointers as possible to g_autofree, then converting a few more *Free* functions (that were more questionable than previous Frees) to use g_free, then some trivial refactoring. And finally at the end a few stragglers that really do need the pointers cleared were changed to g_clear_pointer(x, g_free). A couple of issues: 1) There are two definitions of a macro called ESX_VI__TEMPLATE__FREE that generate functions with names in the form of "esxVI_.*_Free". These generated functions were doing "VIR_FREE(*ptrptr)" at the end; because of the "*", the caller's pointer would be cleared out (not just the local copy). There are something over 400 calls to these functions, and all but 10 or so that I audited will never reference the pointer after return from esxVI_Blah_Free(). But there are several that do. Since I'm unfamiliar with this code and don't want to risk breaking it, I opted to just use g_clear_pointer(ptrptr, g_free), thus preserving current behavior exactly. 2) There are still 7 instances of VIR_FREE in the esx directory. All 7 of them are in loops that are clearing out an array of names prior to returning failure, and from a quick glance it looks like there are at least a few places where it is important to clear the array entries. But rather than brute force convert to using g_clear_pointer in the loop, I figured someone may come up with an elegant translation to use GArray or GPtrArray instead, so I'm leaving them for now. Laine Stump (9): esx: use g_autofree for char* where it is trivially possible esx: use g_autofree when made possible by reducing scope esx: fix memory leak by switching to g_autofree esx: switch VIR_FREE->g_free in esx*Free*() esx: use g_steal_pointer+g_autofree on return value esx: reorder code to avoid need to VIR_FREE mimeType esx: switch VIR_FREE->g_free when the pointer will immediately go out of scope esx: eliminate unnecessary cleanup: labels and result variables esx: replace some VIR_FREE with g_clear_pointer(x, g_free) src/esx/esx_driver.c| 189 +--- src/esx/esx_network_driver.c| 4 +- src/esx/esx_storage_backend_iscsi.c | 16 +-- src/esx/esx_storage_backend_vmfs.c | 150 +++--- src/esx/esx_stream.c| 11 +- src/esx/esx_util.c | 41 +++--- src/esx/esx_vi.c| 111 ++-- src/esx/esx_vi_methods.c| 3 +- src/esx/esx_vi_types.c | 18 +-- 9 files changed, 179 insertions(+), 364 deletions(-) Reviewed-by: Michal Privoznik Michal
[libvirt PATCH 0/9] eliminate (almost) all VIR_FREE in esx directory
142 more down, 3084 to go :-) This goes through all the standard methods of eliminating VIR_FREE - first converting as many pointers as possible to g_autofree, then converting a few more *Free* functions (that were more questionable than previous Frees) to use g_free, then some trivial refactoring. And finally at the end a few stragglers that really do need the pointers cleared were changed to g_clear_pointer(x, g_free). A couple of issues: 1) There are two definitions of a macro called ESX_VI__TEMPLATE__FREE that generate functions with names in the form of "esxVI_.*_Free". These generated functions were doing "VIR_FREE(*ptrptr)" at the end; because of the "*", the caller's pointer would be cleared out (not just the local copy). There are something over 400 calls to these functions, and all but 10 or so that I audited will never reference the pointer after return from esxVI_Blah_Free(). But there are several that do. Since I'm unfamiliar with this code and don't want to risk breaking it, I opted to just use g_clear_pointer(ptrptr, g_free), thus preserving current behavior exactly. 2) There are still 7 instances of VIR_FREE in the esx directory. All 7 of them are in loops that are clearing out an array of names prior to returning failure, and from a quick glance it looks like there are at least a few places where it is important to clear the array entries. But rather than brute force convert to using g_clear_pointer in the loop, I figured someone may come up with an elegant translation to use GArray or GPtrArray instead, so I'm leaving them for now. Laine Stump (9): esx: use g_autofree for char* where it is trivially possible esx: use g_autofree when made possible by reducing scope esx: fix memory leak by switching to g_autofree esx: switch VIR_FREE->g_free in esx*Free*() esx: use g_steal_pointer+g_autofree on return value esx: reorder code to avoid need to VIR_FREE mimeType esx: switch VIR_FREE->g_free when the pointer will immediately go out of scope esx: eliminate unnecessary cleanup: labels and result variables esx: replace some VIR_FREE with g_clear_pointer(x, g_free) src/esx/esx_driver.c| 189 +--- src/esx/esx_network_driver.c| 4 +- src/esx/esx_storage_backend_iscsi.c | 16 +-- src/esx/esx_storage_backend_vmfs.c | 150 +++--- src/esx/esx_stream.c| 11 +- src/esx/esx_util.c | 41 +++--- src/esx/esx_vi.c| 111 ++-- src/esx/esx_vi_methods.c| 3 +- src/esx/esx_vi_types.c | 18 +-- 9 files changed, 179 insertions(+), 364 deletions(-) -- 2.29.2
