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(-)
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
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 <mprivozn@redhat.com> Michal
On Fri, 2021-02-12 at 17:07 -0500, Laine Stump wrote: > 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(-) This appears to have broken building with Clang on Linux: https://gitlab.com/libvirt/libvirt/-/pipelines/257175967 Can you please look into the issue? -- Andrea Bolognani / Red Hat / Virtualization
On a Wednesday in 2021, Andrea Bolognani wrote: >On Fri, 2021-02-12 at 17:07 -0500, Laine Stump wrote: >> 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(-) > >This appears to have broken building with Clang on Linux: > > https://gitlab.com/libvirt/libvirt/-/pipelines/257175967 > >Can you please look into the issue? > It should be fixed by 1f8e6a6172f2df1c4eab1cf2a26488498947641f Jano >-- >Andrea Bolognani / Red Hat / Virtualization >
On Wed, 2021-02-17 at 10:44 +0100, Ján Tomko wrote: > On a Wednesday in 2021, Andrea Bolognani wrote: > > On Fri, 2021-02-12 at 17:07 -0500, Laine Stump wrote: > > > 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(-) > > > > This appears to have broken building with Clang on Linux: > > > > https://gitlab.com/libvirt/libvirt/-/pipelines/257175967 > > > > Can you please look into the issue? > > It should be fixed by 1f8e6a6172f2df1c4eab1cf2a26488498947641f So it is! Nevermind then :) -- Andrea Bolognani / Red Hat / Virtualization
© 2016 - 2024 Red Hat, Inc.