[PATCH 0/2] ch_monitor: Two simple improvements
I've noticed these why reviewing some other CH patches on the list. Michal Prívozník (2): ch_monitor: Avoid possible double free in virCHMonitorClose() ch_monitor: Report OS error when removing socket fails src/ch/ch_monitor.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) -- 2.45.2
The virCHMonitorClose() is meant to be called when monitor to cloud-hypervisor process closes. It removes the socket and frees string containing path to the socket. In general, there is a problem with the following pattern: if (var) { do_something(); g_free(var); } because if the pattern executes twice the variable is freed twice. That's why we have VIR_FREE() macro. Well, replace plain g_free() with g_clear_pointer(). Mind you, this is NOT a destructor where clearing pointers is needless. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index ccd04cfbd1..18ca5a764e 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -626,7 +626,7 @@ void virCHMonitorClose(virCHMonitor *mon) VIR_WARN("Unable to remove CH socket file '%s'", mon->socketpath); } - g_free(mon->socketpath); + g_clear_pointer(&mon->socketpath, g_free); } virObjectUnref(mon); -- 2.45.2
When removing a socket in virCHMonitorClose() fails, a warning is printed. But it doesn't contain errno nor g_strerror() which may shed more light into why removing of the socket failed. Oh, and since virCHMonitorClose() is registered as autoptr cleanup for virCHMonitor() it may happen that virCHMonitorClose() is called with mon->socketpath allocated but file not existing yet (see virCHMonitorNew()). Thus ignore ENOENT and do not print warning in that case - the file doesn't exist anyways. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_monitor.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 18ca5a764e..935239a721 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -622,9 +622,10 @@ void virCHMonitorClose(virCHMonitor *mon) curl_easy_cleanup(mon->handle); if (mon->socketpath) { - if (virFileRemove(mon->socketpath, -1, -1) < 0) { - VIR_WARN("Unable to remove CH socket file '%s'", - mon->socketpath); + if (virFileRemove(mon->socketpath, -1, -1) < 0 && + errno != ENOENT) { + VIR_WARN("Unable to remove CH socket file '%s': %s", + mon->socketpath, g_strerror(errno)); } g_clear_pointer(&mon->socketpath, g_free); } -- 2.45.2
On a Monday in 2024, Michal Privoznik wrote:
I've noticed these why reviewing some other CH patches on the list.
Michal Prívozník (2): ch_monitor: Avoid possible double free in virCHMonitorClose() ch_monitor: Report OS error when removing socket fails
src/ch/ch_monitor.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik