On Mon, Nov 09, 2020 at 15:37:39 +0100, Michal Privoznik wrote:
On 11/6/20 4:32 AM, Matt Coleman wrote:
Signed-off-by: Matt Coleman <matt@datto.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ce49905360..a64dec8df4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5078,7 +5078,7 @@ virDomainPostParseCheckISCSIPath(char **srcpath) return; path = g_strdup_printf("%s/0", *srcpath); - VIR_FREE(*srcpath); + g_free(*srcpath); *srcpath = g_steal_pointer(&path); }
Can't we do this for other places too? I mean, this boils down to discussions we had when starting to adopt glib, but rather than doing this change per function I think we want bigger blocks. The same applies for 20/28 where you're switching to g_renew() from VIR_REALLOC_N(). I understand that you want to touch only some functions because later you are turning their return type into void, but I'd rather see bulk glib conversions.
Specifically you can only replace VIR_FREE with g_free if you immediately overwrite it. Our semantics of VIR_FREE lead in many cases to a code pattern of using VIR_FREE and then reusing the variable and having another VIR_FREE in the cleanup path. Since g_free doesn't clear the pointer this would lead to double frees when blindly replaced. Non-blind replace is obviously very expensive both for the author and for the reviewer. The only blind replacement we can do is to replace VIR_FREE with g_clear_pointer(&ptr, g_free); This is in the end what VIR_FREE exactly does. Now it's only necessary to justify the churn. To me it's borderline not worth it.