[BlueZ v3 0/6] Add helper for "cleanup" variable attribute

[BlueZ v3 0/6] Add helper for "cleanup" variable attribute

[Bastien Nocera]

As discussed in:
https://lore.kernel.org/linux-bluetooth/ed949f2550f79a4bef19bd482bf8b069ad5b7e0c.camel@hadess.net/

Implement a cleanup helper.

The MIN/MAX fix is here because it touches the same hunk in src/main.c
as the other patches. Feel free to pick it up straight away while the
rest is discussed.

Changes since v2:
- Add macros to declare cleanup for specific types
- Add file descriptor cleanup
- Add "steal" helpers for fd and pointers
- Add unit tests

Changes since v1:
- Fixed checkpatch warnings

Bastien Nocera (6):
  all: Remove more unneeded MIN/MAX macro definition
  shared/util: Add helper for "cleanup" variable attribute
  doc: Recommend using _cleanup_ and friends
  main: Use _cleanup_() to simplify configuration parsing
  client: Use _cleanup_fd_ to simplify urandom access
  btattach: Use _cleanup_fd_ to simplify error paths

 client/mgmt.c                 |   5 +-
 doc/maintainer-guidelines.rst |   3 +
 lib/bluetooth/hci.c           |   4 --
 src/main.c                    | 103 ++++++++++------------------------
 src/shared/gatt-server.c      |   8 ---
 src/shared/util.h             |  40 +++++++++++++
 tools/btattach.c              |  18 ++----
 unit/test-util.c              |  65 +++++++++++++++++++++
 8 files changed, 145 insertions(+), 101 deletions(-)

-- 
2.54.0

[BlueZ v3 1/6] all: Remove more unneeded MIN/MAX macro definition

[Bastien Nocera]

---
 lib/bluetooth/hci.c      | 4 ----
 src/main.c               | 4 ----
 src/shared/gatt-server.c | 8 --------
 3 files changed, 16 deletions(-)

diff --git a/lib/bluetooth/hci.c b/lib/bluetooth/hci.c
index 44eea054b0ac..f50a30c610be 100644
--- a/lib/bluetooth/hci.c
+++ b/lib/bluetooth/hci.c
@@ -33,10 +33,6 @@
 #include "hci.h"
 #include "hci_lib.h"
 
-#ifndef MIN
-#define MIN(x, y) ((x) < (y) ? (x) : (y))
-#endif
-
 typedef struct {
 	char *str;
 	unsigned int val;
diff --git a/src/main.c b/src/main.c
index 9a3d2da25d4d..8aa19a3e3346 100644
--- a/src/main.c
+++ b/src/main.c
@@ -205,10 +205,6 @@ static const struct group_table {
 	{ }
 };
 
-#ifndef MIN
-#define MIN(x, y) ((x) < (y) ? (x) : (y))
-#endif
-
 static int8_t check_sirk_alpha_numeric(char *str)
 {
 	int8_t val = 0;
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 6273899965c0..709a8f94bb6a 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -26,14 +26,6 @@
 #include "src/shared/util.h"
 #include "src/shared/timeout.h"
 
-#ifndef MAX
-#define MAX(a, b) ((a) > (b) ? (a) : (b))
-#endif
-
-#ifndef MIN
-#define MIN(a, b) ((a) < (b) ? (a) : (b))
-#endif
-
 /*
  * TODO: This is an arbitrary limit. Come up with something reasonable or
  * perhaps an API to set this value if there is a use case for it.
-- 
2.54.0

RE: Add helper for "cleanup" variable attribute

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 5159 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1092801

---Test result---

Test Summary:
CheckPatch                    FAIL      2.71 seconds
GitLint                       PASS      1.76 seconds
BuildEll                      PASS      20.40 seconds
BluezMake                     PASS      666.47 seconds
MakeCheck                     PASS      18.72 seconds
MakeDistcheck                 PASS      245.36 seconds
CheckValgrind                 PASS      291.26 seconds
CheckSmatch                   WARNING   351.83 seconds
bluezmakeextell               PASS      186.73 seconds
IncrementalBuild              PASS      1330.28 seconds
ScanBuild                     PASS      1038.55 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v3,2/6] shared/util: Add helper for "cleanup" variable attribute
ERROR:POINTER_LOCATION: "foo * bar" should be "foo *bar"
#106: FILE: src/shared/util.h:103:
+static inline void * _steal_(void *p)

WARNING:LONG_LINE: line length of 104 exceeds 80 columns
#132: FILE: src/shared/util.h:129:
+#define CLEANUP_FREEFUNC(type, func) static inline void cleanup_##type (type **_ptr) { (func) (*_ptr); }

WARNING:SPACING: space prohibited between function name and open parenthesis '('
#132: FILE: src/shared/util.h:129:
+#define CLEANUP_FREEFUNC(type, func) static inline void cleanup_##type (type **_ptr) { (func) (*_ptr); }

ERROR:SPACING: need consistent spacing around '*' (ctx:WxO)
#132: FILE: src/shared/util.h:129:
+#define CLEANUP_FREEFUNC(type, func) static inline void cleanup_##type (type **_ptr) { (func) (*_ptr); }
                                                                              ^

WARNING:LINE_SPACING: Missing a blank line after declarations
#184: FILE: unit/test-util.c:46:
+		_cleanup_free_ uint8_t *data = NULL;
+		data = malloc0(128 * 2);

/github/workspace/src/patch/14565706.patch total: 2 errors, 3 warnings, 140 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14565706.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,v3,4/6] main: Use _cleanup_() to simplify configuration parsing
ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#84: FILE: src/main.c:258:
+	_cleanup_type_(GError) GError *err = NULL;
 	                              ^

ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#108: FILE: src/main.c:440:
+	_cleanup_type_(GError) GError *err = NULL;
 	                              ^

ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#204: FILE: src/main.c:888:
+	_cleanup_type_(GError) GError *err = NULL;
 	                              ^

ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#409: FILE: src/main.c:1207:
+	_cleanup_type_(GError) GError *err = NULL;
 	                              ^

ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#501: FILE: src/main.c:1570:
+	_cleanup_type_(GError) GError *err = NULL;
 	                              ^

/github/workspace/src/patch/14565665.patch total: 5 errors, 0 warnings, 413 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14565665.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/main.c: note: in included file (through src/device.h):src/shared/gatt-server.c:271:25: warning: Variable length array is used.src/shared/gatt-server.c:614:25: warning: Variable length array is used.src/shared/gatt-server.c:712:25: warning: Variable length array is used.src/shared/gatt-server.c:271:25: warning: Variable length array is used.src/shared/gatt-server.c:614:25: warning: Variable length array is used.src/shared/gatt-server.c:712:25: warning: Variable length array is used.src/shared/gatt-server.c:271:25: warning: Variable length array is used.src/shared/gatt-server.c:614:25: warning: Variable length array is used.src/shared/gatt-server.c:712:25: warning: Variable length array is used.src/main.c: note: in included file (through src/device.h):


https://github.com/bluez/bluez/pull/2118

---
Regards,
Linux Bluetooth

[BlueZ v3 2/6] shared/util: Add helper for "cleanup" variable attribute

[Bastien Nocera]

Use the widespread "cleanup" variable attribute:
https://gcc.gnu.org/onlinedocs/gcc/Common-Attributes.html#index-cleanup

It is implemented by both GCC and clang on platforms where bluez is
used, and can help reduce memory leaks, while improving readability.

This implements:
- generic cleanup (_cleanup_free_)
- cleanup with specific free function (_cleanup_())
- cleanup for specific types (_cleanup_type_(type))
- cleanup for file descriptors
- capturing a variable before it is freed (so it is only freed in error
  paths for example, _steal_() and _steal_fd())

This commit includes tests which should cover all those new helpers.

See also:
https://systemd.io/CODING_STYLE/#memory-allocation
https://docs.gtk.org/glib/auto-cleanup.html
---
 src/shared/util.h | 40 +++++++++++++++++++++++++++++
 unit/test-util.c  | 65 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/src/shared/util.h b/src/shared/util.h
index 67629dddfaa9..c8ce90c5f1b5 100644
--- a/src/shared/util.h
+++ b/src/shared/util.h
@@ -15,9 +15,11 @@
 #include <stdbool.h>
 #include <alloca.h>
 #include <byteswap.h>
+#include <errno.h>
 #include <string.h>
 #include <sys/types.h>
 #include <sys/uio.h>
+#include <unistd.h>
 
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
 #define BIT(n)  (1 << (n))
@@ -93,6 +95,44 @@ do {						\
 #define newa(t, n) ((t*) alloca(sizeof(t)*(n)))
 #define malloc0(n) (calloc(1, (n)))
 
+static inline void freep(void *p)
+{
+	free(*(void **) p);
+}
+
+static inline void * _steal_(void *p)
+{
+	void **orig = (void **) p;
+	void *ret = *orig;
+	*orig = NULL;
+	return ret;
+}
+
+static inline int _steal_fd_(int *fdp)
+{
+	int fd = *fdp;
+	*fdp = -1;
+	return fd;
+}
+
+static inline void closefd(int *fdp)
+{
+	int errno_save = errno;
+	int fd = *fdp;
+	*fdp = -1;
+	if (fd < 0)
+		return;
+	close(fd);
+	errno = errno_save;
+}
+
+#define CLEANUP_FREEFUNC(type, func) static inline void cleanup_##type (type **_ptr) { (func) (*_ptr); }
+
+#define _cleanup_(f) __attribute__((cleanup(f)))
+#define _cleanup_type_(type) __attribute__((cleanup(cleanup_##type)))
+#define _cleanup_free_ _cleanup_(freep)
+#define _cleanup_fd_ _cleanup_(closefd)
+
 char *strdelimit(char *str, char *del, char c);
 int strsuffix(const char *str, const char *suffix);
 char *strstrip(char *str);
diff --git a/unit/test-util.c b/unit/test-util.c
index 443c4f70362c..1672b32eb39c 100644
--- a/unit/test-util.c
+++ b/unit/test-util.c
@@ -9,14 +9,73 @@
  */
 
 #include <assert.h>
+#include <sys/stat.h>
+#include <fcntl.h>
 
 #include "src/shared/util.h"
 #include "src/shared/tester.h"
+#include "bluetooth/bluetooth.h"
 
 /* XXX glib.h must not be included, or it will clobber the
  * MIN/MAX macros.
  */
 
+static void test_cleanup_free(const void *data)
+{
+	_cleanup_free_ char *p1 = NULL;
+	_cleanup_free_ char *p2 = NULL;
+	_cleanup_free_ char *is_null = NULL;
+
+	p1 = malloc0(10);
+	p2 = malloc0(15);
+
+	p1[0] = 1;
+	p2[0] = 1;
+
+	{
+		_cleanup_free_ uint8_t *data = NULL;
+		_cleanup_free_ uint8_t *is_null_too = NULL;
+
+		data = malloc0(128);
+		data[0] = 1;
+
+		assert(is_null_too == NULL);
+	}
+	{
+		_cleanup_free_ uint8_t *data = NULL;
+		data = malloc0(128 * 2);
+		data[0] = 3;
+	}
+
+	assert(is_null == NULL);
+	tester_test_passed();
+}
+
+CLEANUP_FREEFUNC(bdaddr_t, free);
+
+static void test_cleanup_type(const void *data)
+{
+#define ADDR "FF:FF:FF:FF:FF:FF"
+	_cleanup_type_(bdaddr_t) bdaddr_t *address = NULL;
+	char str[33];
+
+	address = strtoba(ADDR);
+	assert(bacmp(address, BDADDR_ALL) == 0);
+	printf("%d = ba2str(address, str)\n", ba2str(address, str));
+	assert(ba2str(address, str) == 17);
+	assert(strcmp(str, ADDR) == 0);
+	tester_test_passed();
+}
+
+static void test_cleanup_fd(const void *data)
+{
+	_cleanup_fd_ int fd = -1;
+
+	fd = open("/dev/null", O_RDONLY);
+	assert(fd != 0);
+	tester_test_passed();
+}
+
 static void test_min_max(const void *data)
 {
 	assert(MIN(3, 4) == 3);
@@ -30,6 +89,12 @@ int main(int argc, char *argv[])
 
 	tester_add("/util/min_max", NULL, NULL,
 			test_min_max, NULL);
+	tester_add("/util/cleanup_free", NULL, NULL,
+			test_cleanup_free, NULL);
+	tester_add("/util/cleanup_type", NULL, NULL,
+			test_cleanup_type, NULL);
+	tester_add("/util/cleanup_fd", NULL, NULL,
+			test_cleanup_fd, NULL);
 
 	return tester_run();
 }
-- 
2.54.0

[BlueZ v3 3/6] doc: Recommend using _cleanup_ and friends

[Bastien Nocera]

---
 doc/maintainer-guidelines.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/doc/maintainer-guidelines.rst b/doc/maintainer-guidelines.rst
index 44d3e258db6e..b67c6596f4c1 100644
--- a/doc/maintainer-guidelines.rst
+++ b/doc/maintainer-guidelines.rst
@@ -98,6 +98,9 @@ do this:
 
 The above assumes that a kernel tree resides in ``~/src/linux/``.
 
+Also make sure to use ``_cleanup_free_`` and ``_cleanup_(free_func)`` when
+possible. It makes your code much nicer to read (and shorter), and avoids
+common memory leaks on error paths.
 
 Rule 4: Pay extra attention to adding new files to the tree
 -----------------------------------------------------------
-- 
2.54.0

[BlueZ v3 4/6] main: Use _cleanup_() to simplify configuration parsing

[Bastien Nocera]

Use helpers to simplify temporary string usage, and cleanup in error
paths.
---
 src/main.c | 101 ++++++++++++++++-------------------------------------
 1 file changed, 31 insertions(+), 70 deletions(-)

diff --git a/src/main.c b/src/main.c
index 8aa19a3e3346..9053e74aaf75 100644
--- a/src/main.c
+++ b/src/main.c
@@ -205,6 +205,9 @@ static const struct group_table {
 	{ }
 };
 
+CLEANUP_FREEFUNC(GError, g_error_free);
+CLEANUP_FREEFUNC(GKeyFile, g_key_file_free);
+
 static int8_t check_sirk_alpha_numeric(char *str)
 {
 	int8_t val = 0;
@@ -252,8 +255,8 @@ GKeyFile *btd_get_main_conf(void)
 
 static GKeyFile *load_config(const char *name)
 {
-	GError *err = NULL;
-	GKeyFile *keyfile;
+	_cleanup_type_(GError) GError *err = NULL;
+	_cleanup_type_(GKeyFile) GKeyFile *keyfile = NULL;
 	int len;
 
 	if (name)
@@ -285,12 +288,10 @@ static GKeyFile *load_config(const char *name)
 		if (!g_error_matches(err, G_FILE_ERROR, G_FILE_ERROR_NOENT))
 			error("Parsing %s failed: %s", main_conf_file_path,
 				err->message);
-		g_error_free(err);
-		g_key_file_free(keyfile);
 		return NULL;
 	}
 
-	return keyfile;
+	return _steal_(keyfile);
 }
 
 static void parse_did(const char *did)
@@ -436,14 +437,13 @@ static int get_mode(const char *str)
 static bool parse_config_string(GKeyFile *config, const char *group,
 					const char *key, char **val)
 {
-	GError *err = NULL;
+	_cleanup_type_(GError) GError *err = NULL;
 	char *tmp;
 
 	tmp = g_key_file_get_string(config, group, key, &err);
 	if (err) {
 		if (err->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND)
 			DBG("%s", err->message);
-		g_error_free(err);
 		return false;
 	}
 
@@ -462,7 +462,7 @@ static bool parse_config_int(GKeyFile *config, const char *group,
 					size_t min, size_t max)
 {
 	size_t tmp;
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 	char *endptr = NULL;
 
 	if (!parse_config_string(config, group, key, &str))
@@ -471,25 +471,21 @@ static bool parse_config_int(GKeyFile *config, const char *group,
 	tmp = strtol(str, &endptr, 0);
 	if (!endptr || *endptr != '\0') {
 		error("%s.%s = %s is not integer", group, key, str);
-		g_free(str);
 		return false;
 	}
 
 	if (tmp < min) {
-		g_free(str);
 		warn("%s.%s = %zu is out of range (< %zu)", group, key, tmp,
 									min);
 		return false;
 	}
 
 	if (tmp > max) {
-		g_free(str);
 		warn("%s.%s = %zu is out of range (> %zu)", group, key, tmp,
 									max);
 		return false;
 	}
 
-	g_free(str);
 	if (val)
 		*val = tmp;
 
@@ -500,10 +496,9 @@ static bool parse_config_signed_int(GKeyFile *config, const char *group,
 					const char *key, int8_t *val,
 					long min, long max)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 	char *endptr = NULL;
 	long tmp;
-	bool result = false;
 
 	str = g_key_file_get_string(config, group, key, NULL);
 	if (!str)
@@ -512,28 +507,24 @@ static bool parse_config_signed_int(GKeyFile *config, const char *group,
 	tmp = strtol(str, &endptr, 0);
 	if (!endptr || *endptr != '\0') {
 		warn("%s.%s = %s is not integer", group, key, str);
-		goto cleanup;
+		return false;
 	}
 
 	if (tmp < min) {
 		warn("%s.%s = %ld is out of range (< %ld)", group, key, tmp,
 			min);
-		goto cleanup;
+		return false;
 	}
 
 	if (tmp > max) {
 		warn("%s.%s = %ld is out of range (> %ld)", group, key, tmp,
 									max);
-		goto cleanup;
+		return false;
 	}
 
 	if (val)
 		*val = (int8_t) tmp;
-	result = true;
-
-cleanup:
-	g_free(str);
-	return result;
+	return true;
 }
 
 struct config_param {
@@ -894,14 +885,13 @@ static bool parse_config_u8(GKeyFile *config, const char *group,
 static bool parse_config_bool(GKeyFile *config, const char *group,
 					const char *key, bool *val)
 {
-	GError *err = NULL;
+	_cleanup_type_(GError) GError *err = NULL;
 	gboolean tmp;
 
 	tmp = g_key_file_get_boolean(config, group, key, &err);
 	if (err) {
 		if (err->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND)
 			DBG("%s", err->message);
-		g_error_free(err);
 		return false;
 	}
 
@@ -915,7 +905,7 @@ static bool parse_config_bool(GKeyFile *config, const char *group,
 
 static void parse_privacy(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	if (!parse_config_string(config, "General", "Privacy", &str)) {
 		btd_opts.privacy = 0x00;
@@ -948,13 +938,11 @@ static void parse_privacy(GKeyFile *config)
 		DBG("Invalid privacy option: %s", str);
 		btd_opts.privacy = 0x00;
 	}
-
-	g_free(str);
 }
 
 static void parse_repairing(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	if (!parse_config_string(config, "General", "JustWorksRepairing",
 						&str)) {
@@ -963,13 +951,12 @@ static void parse_repairing(GKeyFile *config)
 	}
 
 	btd_opts.jw_repairing = parse_jw_repairing(str);
-	g_free(str);
 }
 
 static bool parse_config_hex(GKeyFile *config, char *group,
 					const char *key, uint32_t *val)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	if (!parse_config_string(config, group, key, &str))
 		return false;
@@ -977,37 +964,34 @@ static bool parse_config_hex(GKeyFile *config, char *group,
 	if (val)
 		*val = strtol(str, NULL, 16);
 
-	g_free(str);
 	return true;
 }
 
 static void parse_device_id(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	parse_config_string(config, "General", "DeviceID", &str);
 	if (!str)
 		return;
 
 	parse_did(str);
-	g_free(str);
 }
 
 static void parse_ctrl_mode(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	parse_config_string(config, "General", "ControllerMode", &str);
 	if (!str)
 		return;
 
 	btd_opts.mode = get_mode(str);
-	g_free(str);
 }
 
 static void parse_multi_profile(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	parse_config_string(config, "General", "MultiProfile", &str);
 	if (!str)
@@ -1019,8 +1003,6 @@ static void parse_multi_profile(GKeyFile *config)
 		btd_opts.mps = MPS_MULTIPLE;
 	else
 		btd_opts.mps = MPS_OFF;
-
-	g_free(str);
 }
 
 static gboolean parse_kernel_experimental(const char *key, const char *value,
@@ -1043,20 +1025,18 @@ static gboolean parse_kernel_experimental(const char *key, const char *value,
 
 static void parse_kernel_exp(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	if (!parse_config_string(config, "General", "KernelExperimental",
 						&str))
 		return;
 
 	parse_kernel_experimental(NULL, str, NULL, NULL);
-
-	g_free(str);
 }
 
 static void parse_secure_conns(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	if (!parse_config_string(config, "General", "SecureConnections",
 								&str))
@@ -1068,8 +1048,6 @@ static void parse_secure_conns(GKeyFile *config)
 		btd_opts.secure_conn = SC_ON;
 	else if (!strcmp(str, "only"))
 		btd_opts.secure_conn = SC_ONLY;
-
-	g_free(str);
 }
 
 static void parse_general(GKeyFile *config)
@@ -1120,14 +1098,13 @@ static void parse_general(GKeyFile *config)
 
 static void parse_gatt_cache(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	parse_config_string(config, "GATT", "Cache", &str);
 	if (!str)
 		return;
 
 	btd_opts.gatt_cache = parse_gatt_cache_str(str);
-	g_free(str);
 }
 
 static enum bt_gatt_export_t parse_gatt_export_str(const char *str)
@@ -1147,14 +1124,13 @@ static enum bt_gatt_export_t parse_gatt_export_str(const char *str)
 
 static void parse_gatt_export(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	parse_config_string(config, "GATT", "ExportClaimedServices", &str);
 	if (!str)
 		return;
 
 	btd_opts.gatt_export = parse_gatt_export_str(str);
-	g_free(str);
 }
 
 static uint8_t parse_gatt_seclevel_str(const char *str)
@@ -1176,7 +1152,7 @@ static uint8_t parse_gatt_seclevel_str(const char *str)
 
 static void parse_gatt_seclevel(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	if (!btd_opts.testing)
 		return;
@@ -1186,7 +1162,6 @@ static void parse_gatt_seclevel(GKeyFile *config)
 		return;
 
 	btd_opts.gatt_seclevel = parse_gatt_seclevel_str(str);
-	g_free(str);
 }
 
 static void parse_gatt(GKeyFile *config)
@@ -1204,7 +1179,7 @@ static void parse_gatt(GKeyFile *config)
 
 static void parse_csis_sirk(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	if (!parse_config_string(config, "CSIS", "SIRK", &str))
 		return;
@@ -1213,8 +1188,6 @@ static void parse_csis_sirk(GKeyFile *config)
 		hex2bin(str, btd_opts.csis.sirk, sizeof(btd_opts.csis.sirk));
 	else if (!gen_sirk(str))
 		DBG("Unable to generate SIRK from string");
-
-	g_free(str);
 }
 
 static void parse_csis(GKeyFile *config)
@@ -1231,8 +1204,8 @@ static void parse_csis(GKeyFile *config)
 static bool parse_cs_role(GKeyFile *config, const char *group,
 					const char *key, uint8_t *val)
 {
-	GError *err = NULL;
-	char *str = NULL;
+	_cleanup_type_(GError) GError *err = NULL;
+	_cleanup_(g_free) char *str = NULL;
 	char *endptr = NULL;
 	int numeric_val;
 
@@ -1241,7 +1214,6 @@ static bool parse_cs_role(GKeyFile *config, const char *group,
 	if (err) {
 		if (err->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND)
 			DBG("%s", err->message);
-		g_error_free(err);
 		return false;
 	}
 
@@ -1251,17 +1223,14 @@ static bool parse_cs_role(GKeyFile *config, const char *group,
 	if (!strcmp(str, "Initiator") || !strcmp(str, "initiator")) {
 		if (val)
 			*val = 1;
-		g_free(str);
 		return true;
 	} else if (!strcmp(str, "Reflector") || !strcmp(str, "reflector")) {
 		if (val)
 			*val = 2;
-		g_free(str);
 		return true;
 	} else if (!strcmp(str, "Both") || !strcmp(str, "both")) {
 		if (val)
 			*val = 3;
-		g_free(str);
 		return true;
 	}
 
@@ -1272,7 +1241,6 @@ static bool parse_cs_role(GKeyFile *config, const char *group,
 		warn("%s.%s = %s is not a valid value. "
 			"Expected: 1/Initiator, 2/Reflector, or 3/Both",
 			group, key, str);
-		g_free(str);
 		return false;
 	}
 
@@ -1280,14 +1248,12 @@ static bool parse_cs_role(GKeyFile *config, const char *group,
 		warn("%s.%s = %d is out of range. "
 			"Valid values: 1 (Initiator), 2 (Reflector), 3 (Both)",
 			group, key, numeric_val);
-		g_free(str);
 		return false;
 	}
 
 	if (val)
 		*val = numeric_val;
 
-	g_free(str);
 	return true;
 }
 
@@ -1305,7 +1271,7 @@ static void parse_le_cs_config(GKeyFile *config)
 
 static void parse_avdtp_session_mode(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	if (!parse_config_string(config, "AVDTP", "SessionMode", &str))
 		return;
@@ -1318,13 +1284,11 @@ static void parse_avdtp_session_mode(GKeyFile *config)
 		DBG("Invalid mode option: %s", str);
 		btd_opts.avdtp.session_mode = BT_IO_MODE_BASIC;
 	}
-
-	g_free(str);
 }
 
 static void parse_avdtp_stream_mode(GKeyFile *config)
 {
-	char *str = NULL;
+	_cleanup_(g_free) char *str = NULL;
 
 	if (!parse_config_string(config, "AVDTP", "StreamMode", &str))
 		return;
@@ -1337,8 +1301,6 @@ static void parse_avdtp_stream_mode(GKeyFile *config)
 		DBG("Invalid mode option: %s", str);
 		btd_opts.avdtp.stream_mode = BT_IO_MODE_BASIC;
 	}
-
-	g_free(str);
 }
 
 static void parse_avdtp(GKeyFile *config)
@@ -1605,7 +1567,7 @@ static GOptionEntry options[] = {
 int main(int argc, char *argv[])
 {
 	GOptionContext *context;
-	GError *err = NULL;
+	_cleanup_type_(GError) GError *err = NULL;
 	uint16_t sdp_mtu = 0;
 	uint32_t sdp_flags = 0;
 	int gdbus_flags = 0;
@@ -1618,7 +1580,6 @@ int main(int argc, char *argv[])
 	if (g_option_context_parse(context, &argc, &argv, &err) == FALSE) {
 		if (err != NULL) {
 			g_printerr("%s\n", err->message);
-			g_error_free(err);
 		} else
 			g_printerr("An unknown error occurred\n");
 		exit(1);
-- 
2.54.0

RE: [BlueZ,v3,4/6] main: Use _cleanup_() to simplify configuration parsing

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: src/main.c:205
error: src/main.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

[BlueZ v3 5/6] client: Use _cleanup_fd_ to simplify urandom access

[Bastien Nocera]

fd gets auto-closed before exiting the scope.
---
 client/mgmt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/client/mgmt.c b/client/mgmt.c
index 50558a313866..e199c30540e4 100644
--- a/client/mgmt.c
+++ b/client/mgmt.c
@@ -2681,7 +2681,7 @@ static void cmd_privacy(int argc, char **argv)
 			return bt_shell_noninteractive_quit(EXIT_FAILURE);
 		}
 	} else {
-		int fd;
+		_cleanup_fd_ int fd = -1;
 
 		fd = open("/dev/urandom", O_RDONLY);
 		if (fd < 0) {
@@ -2691,11 +2691,8 @@ static void cmd_privacy(int argc, char **argv)
 
 		if (read(fd, cp.irk, sizeof(cp.irk)) != sizeof(cp.irk)) {
 			error("Reading from urandom failed");
-			close(fd);
 			return bt_shell_noninteractive_quit(EXIT_FAILURE);
 		}
-
-		close(fd);
 	}
 
 	if (send_cmd(mgmt, MGMT_OP_SET_PRIVACY, index, sizeof(cp), &cp,
-- 
2.54.0

[BlueZ v3 6/6] btattach: Use _cleanup_fd_ to simplify error paths

[Bastien Nocera]

Use _cleanup_fd_ and _steal_fd() to simplify error paths, and only
"steal" the file descriptor on success.
---
 tools/btattach.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/tools/btattach.c b/tools/btattach.c
index 5f7d19093698..8868871b0128 100644
--- a/tools/btattach.c
+++ b/tools/btattach.c
@@ -40,7 +40,8 @@
 static int open_serial(const char *path, unsigned int speed, bool flowctl)
 {
 	struct termios ti;
-	int fd, saved_ldisc, ldisc = N_HCI;
+	_cleanup_fd_ int fd = -1;
+	int saved_ldisc, ldisc = N_HCI;
 
 	fd = open(path, O_RDWR | O_NOCTTY);
 	if (fd < 0) {
@@ -50,13 +51,11 @@ static int open_serial(const char *path, unsigned int speed, bool flowctl)
 
 	if (tcflush(fd, TCIOFLUSH) < 0) {
 		perror("Failed to flush serial port");
-		close(fd);
 		return -1;
 	}
 
 	if (ioctl(fd, TIOCGETD, &saved_ldisc) < 0) {
 		perror("Failed get serial line discipline");
-		close(fd);
 		return -1;
 	}
 
@@ -73,19 +72,17 @@ static int open_serial(const char *path, unsigned int speed, bool flowctl)
 
 	if (tcsetattr(fd, TCSANOW, &ti) < 0) {
 		perror("Failed to set serial port settings");
-		close(fd);
 		return -1;
 	}
 
 	if (ioctl(fd, TIOCSETD, &ldisc) < 0) {
 		perror("Failed set serial line discipline");
-		close(fd);
 		return -1;
 	}
 
 	printf("Switched line discipline from %d to %d\n", saved_ldisc, ldisc);
 
-	return fd;
+	return _steal_fd_(&fd);
 }
 
 static void local_version_callback(const void *data, uint8_t size,
@@ -99,7 +96,8 @@ static void local_version_callback(const void *data, uint8_t size,
 static int attach_proto(const char *path, unsigned int proto,
 			unsigned int speed, bool flowctl, unsigned int flags)
 {
-	int fd, dev_id;
+	_cleanup_fd_ int fd = -1;
+	int dev_id;
 
 	fd = open_serial(path, speed, flowctl);
 	if (fd < 0)
@@ -107,20 +105,17 @@ static int attach_proto(const char *path, unsigned int proto,
 
 	if (ioctl(fd, HCIUARTSETFLAGS, flags) < 0) {
 		perror("Failed to set flags");
-		close(fd);
 		return -1;
 	}
 
 	if (ioctl(fd, HCIUARTSETPROTO, proto) < 0) {
 		perror("Failed to set protocol");
-		close(fd);
 		return -1;
 	}
 
 	dev_id = ioctl(fd, HCIUARTGETDEVICE);
 	if (dev_id < 0) {
 		perror("Failed to get device id");
-		close(fd);
 		return -1;
 	}
 
@@ -140,7 +135,6 @@ static int attach_proto(const char *path, unsigned int proto,
 
 		if (!hci) {
 			fprintf(stderr, "Failed to open HCI user channel\n");
-			close(fd);
 			return -1;
 		}
 
@@ -149,7 +143,7 @@ static int attach_proto(const char *path, unsigned int proto,
 					(bt_hci_destroy_func_t) bt_hci_unref);
 	}
 
-	return fd;
+	return _steal_fd_(&fd);
 }
 
 static void uart_callback(int fd, uint32_t events, void *user_data)
-- 
2.54.0

Re: [BlueZ v3 0/6] Add helper for "cleanup" variable attribute

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon, 11 May 2026 15:18:03 +0200 you wrote:
> As discussed in:
> https://lore.kernel.org/linux-bluetooth/ed949f2550f79a4bef19bd482bf8b069ad5b7e0c.camel@hadess.net/
> 
> Implement a cleanup helper.
> 
> The MIN/MAX fix is here because it touches the same hunk in src/main.c
> as the other patches. Feel free to pick it up straight away while the
> rest is discussed.
> 
> [...]

Here is the summary with links:
  - [BlueZ,v3,1/6] all: Remove more unneeded MIN/MAX macro definition
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=ca2b39b0c08e
  - [BlueZ,v3,2/6] shared/util: Add helper for "cleanup" variable attribute
    (no matching commit)
  - [BlueZ,v3,3/6] doc: Recommend using _cleanup_ and friends
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9ba6c13df5fb
  - [BlueZ,v3,4/6] main: Use _cleanup_() to simplify configuration parsing
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=140e3569295c
  - [BlueZ,v3,5/6] client: Use _cleanup_fd_ to simplify urandom access
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=e33f5027b898
  - [BlueZ,v3,6/6] btattach: Use _cleanup_fd_ to simplify error paths
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=02aa9a8cfe6a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html