From patchwork Thu Jul 25 08:43:38 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Daniel_P=2E_Berrang=C3=A9?= X-Patchwork-Id: 11058245 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 7094F112C for ; Thu, 25 Jul 2019 08:43:59 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5CCDA2893D for ; Thu, 25 Jul 2019 08:43:59 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4DCBC2894C; Thu, 25 Jul 2019 08:43:59 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id DEB072893D for ; Thu, 25 Jul 2019 08:43:58 +0000 (UTC) Received: from localhost ([::1]:57146 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hqZLy-0005FA-3p for patchwork-qemu-devel@patchwork.kernel.org; Thu, 25 Jul 2019 04:43:58 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:49955) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hqZLp-0004qm-PE for qemu-devel@nongnu.org; Thu, 25 Jul 2019 04:43:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hqZLo-0008Fy-I8 for qemu-devel@nongnu.org; Thu, 25 Jul 2019 04:43:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41352) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hqZLo-0008EK-Ag for qemu-devel@nongnu.org; Thu, 25 Jul 2019 04:43:48 -0400 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E8FD731628FF for ; Thu, 25 Jul 2019 08:43:45 +0000 (UTC) Received: from localhost.localdomain.com (ovpn-112-56.ams2.redhat.com [10.36.112.56]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9F4261017E30; Thu, 25 Jul 2019 08:43:43 +0000 (UTC) From: =?utf-8?q?Daniel_P=2E_Berrang=C3=A9?= To: qemu-devel@nongnu.org Date: Thu, 25 Jul 2019 09:43:38 +0100 Message-Id: <20190725084341.8287-1-berrange@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.41]); Thu, 25 Jul 2019 08:43:45 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH v2 0/3] require newer glib2 to enable autofree'ing of stack variables exiting scope X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: =?utf-8?q?Daniel_P=2E_Berrang=C3=A9?= Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP Both GCC and CLang support a C extension attribute((cleanup)) which allows you to define a function that is invoked when a stack variable exits scope. This typically used to free the memory allocated to it, though you're not restricted to this. For example it could be used to unlock a mutex. We could use that functionality now, but the syntax is a bit ugly in plain C. Since version 2.44 of GLib, there have been a few macros to make it more friendly to use - g_autofree, g_autoptr and G_DEFINE_AUTOPTR_CLEANUP_FUNC. https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html https://blogs.gnome.org/desrt/2015/01/30/g_autoptr/ The key selling point is that it simplifies the cleanup code paths, often eliminating the need to goto cleanup labels. This improves the readability of the code and makes it less likely that you'll leak memory accidentally. Inspired by seeing it added to glib, and used in systemd, Libvirt finally got around to adopting this in Feb 2019. Overall our experience with it has been favourable/positive, with the code simplification being very nice. The main caveats with it are - Only works with GCC or CLang. We don't care as those are the only two compilers we declare support for. - You must always initialize the variables when declared to ensure predictable behaviour when they leave scope. Chances are most methods with goto jumps for cleanup are doing this already - You must not directly return the value that's assigned to a auto-cleaned variable. You must steal the pointer in some way. ie BAD: g_autofree char *wibble = g_strdup("wibble") .... return wibble; GOOD: g_autofree char *wibble = g_strdup("wibble") ... return g_steal_pointer(wibble); g_steal_pointer is an inline function which simply copies the pointer to a new variable, and sets the original variable to NULL, thus avoiding cleanup. I've illustrated the usage by converting a bunch of the crypto code in QEMU to use auto cleanup. Changed on v2: - Actually commit the rest of the changes to patch 3 so that what's posted works :-) Sigh. Daniel P. Berrangé (3): glib: bump min required glib library version to 2.48 crypto: define cleanup functions for use with g_autoptr crypto: use auto cleanup for many stack variables configure | 2 +- crypto/afsplit.c | 28 ++++---------- crypto/block-luks.c | 74 +++++++++++-------------------------- crypto/block.c | 15 +++----- crypto/hmac-glib.c | 5 --- crypto/pbkdf.c | 5 +-- crypto/secret.c | 38 ++++++++----------- crypto/tlscredsanon.c | 16 +++----- crypto/tlscredspsk.c | 5 +-- crypto/tlscredsx509.c | 16 +++----- include/crypto/block.h | 2 + include/crypto/cipher.h | 2 + include/crypto/hmac.h | 2 + include/crypto/ivgen.h | 2 + include/crypto/tlssession.h | 2 + include/glib-compat.h | 42 +-------------------- 16 files changed, 78 insertions(+), 178 deletions(-) Reviewed-by: Marc-André Lureau